(fix/solve) Solve ofac.json path issue #244
Conversation
WalkthroughThe changes involve the Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
PavelInjective
force-pushed
the
ofac-fix
branch
from
7306a22 to
b6de017
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
client/chain/ofac.go (1)
Line range hint
26-30: Update callers ofgetOfacListPathIf
getOfacListPathis modified to return an error as suggested in the previous comment, the following functions need to be updated to handle this error:
NewOfacCheckerDownloadOfacListHere are the suggested changes:
For
NewOfacChecker:func NewOfacChecker() (*OfacChecker, error) { ofacListPath, err := getOfacListPath() if err != nil { return nil, fmt.Errorf("failed to get OFAC list path: %w", err) } checker := &OfacChecker{ ofacListPath: ofacListPath, } // ... rest of the function }For
DownloadOfacList:func DownloadOfacList() error { // ... existing code ... ofacListPath, err := getOfacListPath() if err != nil { return fmt.Errorf("failed to get OFAC list path: %w", err) } outFile, err := os.Create(ofacListPath) if err != nil { return err } // ... rest of the function }These changes will ensure proper error handling throughout the OFAC checking process.
Also applies to: 48-52
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- client/chain/ofac.go (1 hunks)
Additional comments not posted (2)
client/chain/ofac.go (2)
Line range hint
1-93: General improvements for OFAC handlingAfter reviewing the entire file, here are some suggestions for improvement:
Make the OFAC list URL configurable:
Instead of hardcodingdefaultOfacListURL, consider using an environment variable or a configuration file. This would allow for easier updates and environment-specific URLs.var ofacListURL = os.Getenv("OFAC_LIST_URL") if ofacListURL == "" { ofacListURL = "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/master/wallets/ofac.json" }Use more inclusive terminology:
ReplaceIsBlacklistedwith a more neutral term likeIsRestrictedorIsOnSanctionsList.func (oc *OfacChecker) IsRestricted(address string) bool { return oc.ofacList[address] }Implement periodic updates:
Consider adding a goroutine that periodically checks for updates to the OFAC list and refreshes the local copy if necessary.These changes would improve the flexibility, inclusivity, and up-to-date nature of the OFAC checking system.
Let's verify the current update mechanism and usage of the
IsBlacklistedfunction:#!/bin/bash # Description: Check for update mechanisms and usage of IsBlacklisted # Test 1: Look for any update mechanisms echo "Searching for update mechanisms:" rg --type go "update.*ofac" # Test 2: Check usage of IsBlacklisted echo "Usage of IsBlacklisted:" rg --type go "IsBlacklisted"
43-43: Simplification introduces potential fragilityThe simplified
getOfacListPathfunction removes the previous directory traversal logic, which makes the code cleaner but potentially less robust. Consider the following points:
The function now assumes a fixed directory structure relative to the current working directory. This might break if:
- The function is called from a different working directory.
- The project structure changes.
There's no error handling for
os.Getwd(), which could fail in certain environments.To improve robustness, consider:
- Using an absolute path or an environment variable to locate the OFAC list file.
- Adding error handling for
os.Getwd().Here's a potential improvement:
func getOfacListPath() (string, error) { currentDirectory, err := os.Getwd() if err != nil { return "", fmt.Errorf("failed to get current directory: %w", err) } return filepath.Join(currentDirectory, "../metadata", ofacListFilename), nil }This change would require updating the callers of
getOfacListPathto handle the potential error.Let's verify the usage of
getOfacListPathin the codebase:Verification successful
[/run_scripts]
[/run_scripts]
[/run_scripts]
Path to OFAC list is correctly defined and accessible
The
getOfacListPathfunction accurately points toclient/metadata/ofac.json, which exists in the repository. No issues were found with the current implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of getOfacListPath and potential impacts # Test 1: Find all occurrences of getOfacListPath echo "Occurrences of getOfacListPath:" rg --type go "getOfacListPath" # Test 2: Check if there are any other files in the metadata directory echo "Files in ../metadata directory:" fd . "../metadata"Length of output: 650
Script:
#!/bin/bash # Description: Check usage of getOfacListPath and potential impacts # Test 1: Find all occurrences of getOfacListPath echo "Occurrences of getOfacListPath:" rg --type go "getOfacListPath" # Test 2: Check if there are any other files in the metadata directory echo "Files in ../metadata directory:" fd . "../metadata"Length of output: 604
Script:
#!/bin/bash # Description: Locate all 'metadata' directories and list their contents # Test 1: Find all directories named 'metadata' echo "Searching for all 'metadata' directories in the repository:" fd metadata --type d # Test 2: For each found 'metadata' directory, list its contents echo "Listing contents of each 'metadata' directory:" fd metadata --type d -x ls -la {}Length of output: 1739
Script:
#!/bin/bash # Description: Locate the definition of 'ofacListFilename' and verify its usage # Test 1: Find the definition of 'ofacListFilename' echo "Searching for the definition of 'ofacListFilename':" rg --type go "ofacListFilename\s*=" # Test 2: Find all occurrences of 'ofacListFilename' to understand its usage echo "Occurrences of 'ofacListFilename' in the codebase:" rg --type go "ofacListFilename" # Test 3: Verify if 'ofac.json' exists in 'client/metadata/' directory echo "Checking for 'ofac.json' in 'client/metadata/':" fd ofac.json client/metadata/ # Test 4: List all 'ofac.json' files in the repository echo "Listing all 'ofac.json' files in the repository:" fd ofac.json --type fLength of output: 1272
PavelInjective
force-pushed
the
ofac-fix
branch
from
019a035 to
fef78fa
Compare
There was a problem hiding this comment.
I am adding just one comment.
Also @PavelInjective I notice that in this PR you did a force push. I already asked in the past to please avoid force pushes. From now one if a PR includes a force push, I will directly reject it.
client/chain/ofac.go
Outdated
| currentDirectory = filepath.Dir(currentDirectory) | ||
| } | ||
| return filepath.Join(filepath.Join(filepath.Join(currentDirectory, "client"), "metadata"), ofacListFilename) | ||
| return getFileAbsPath(fmt.Sprintf("../metadata/%s", ofacListFilename)) |
There was a problem hiding this comment.
We should not make assumptions regarding the directory separator used by the OS. To create a path we should be using path.Join
Summary by CodeRabbit