Fix: update OFAC filename #266
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
💤 Files with no reviewable 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
|
![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 (2)
client/chain/ofac.go (2)
Line range hint
26-36: Consider adding migration handling for existing installationsThe change in filename might affect existing installations that have the old
ofac.jsonfile. Consider:
- Adding a fallback mechanism to check for the old filename
- Adding a migration step to handle the transition
Here's a suggested improvement:
func NewOfacChecker() (*OfacChecker, error) { checker := &OfacChecker{ ofacListPath: GetOfacListPath(), } if _, err := os.Stat(checker.ofacListPath); os.IsNotExist(err) { + // Check for old filename + oldPath := path.Join(OfacListPath, "ofac.json") + if _, err := os.Stat(oldPath); err == nil { + // Migrate old file + if err := os.Rename(oldPath, checker.ofacListPath); err != nil { + return nil, err + } + } else { if err := DownloadOfacList(); err != nil { return nil, err } + } } if err := checker.loadOfacList(); err != nil { return nil, err } return checker, nil }
Line range hint
41-63: Enhance security measures for OFAC list downloadsThe current implementation downloads the OFAC list without verifying its authenticity. Consider implementing:
- Checksum verification
- Digital signature validation
- Rate limiting for downloads
- Backup URL support
Here's a suggested security enhancement:
+const ( + DefaultOfacListURL = "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofacAndRestricted.json" + // Add backup URLs and expected checksum + BackupOfacListURL = "https://backup-domain.com/ofacAndRestricted.json" + ExpectedChecksum = "<SHA256_HASH>" +) func DownloadOfacList() error { + // Implement exponential backoff and rate limiting + urls := []string{DefaultOfacListURL, BackupOfacListURL} + var lastErr error + + for _, url := range urls { resp, err := http.Get(url) if err != nil { + lastErr = err + continue } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { + lastErr = fmt.Errorf("failed to download OFAC list from %s, status code: %d", url, resp.StatusCode) + continue } + // Verify checksum + hash := sha256.New() + tee := io.TeeReader(resp.Body, hash) if err := os.MkdirAll(OfacListPath, 0755); err != nil { return err } outFile, err := os.Create(GetOfacListPath()) if err != nil { return err } defer outFile.Close() - _, err = io.Copy(outFile, resp.Body) + _, err = io.Copy(outFile, tee) if err != nil { return err } + // Verify checksum + if hex.EncodeToString(hash.Sum(nil)) != ExpectedChecksum { + return fmt.Errorf("checksum verification failed for %s", url) + } return nil } - return nil + return fmt.Errorf("failed to download OFAC list from all sources: %v", lastErr) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
client/chain/ofac.go(1 hunks)
🔇 Additional comments (1)
client/chain/ofac.go (1)
13-13: Verify the new OFAC list URL and data format compatibility
The changes expand the scope to include both OFAC and restricted wallets. We should verify:
- The new URL is accessible and returns valid data
- The data format remains compatible with the existing JSON unmarshaling logic
Also applies to: 18-18
✅ Verification successful
Let me verify the code that processes this JSON data to ensure it's compatible with the format.
Let me try a different approach to find the JSON processing code.
Let me check the complete implementation of the OFAC list loading and processing.
URL and data format changes are compatible and safe
The verification confirms:
- The new URL is accessible and returns valid JSON
- The data format is compatible with the existing code implementation:
loadOfacList()expects a JSON array of strings, which matches the format verified from the URL- The unmarshaling and processing logic remains unchanged, only handling the list of addresses regardless of whether they are OFAC or restricted addresses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new OFAC list URL and data format
# Test 1: Check if the URL is accessible and returns valid JSON
echo "Checking URL accessibility and JSON validity..."
curl -s "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofacAndRestricted.json" | jq . >/dev/null
if [ $? -eq 0 ]; then
echo "URL returns valid JSON"
else
echo "Error: URL does not return valid JSON"
exit 1
fi
# Test 2: Verify the data structure matches expected format (array of strings)
echo "Verifying data structure..."
curl -s "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofacAndRestricted.json" | jq 'if type == "array" and (.[0] | type == "string") then "Valid format" else "Invalid format" end'
Length of output: 622
Script:
#!/bin/bash
# Find and examine the code that processes the OFAC list JSON
ast-grep --pattern 'func $_($$$ *ofac.json$$$) {
$$$
}'
# Also search for any direct JSON unmarshaling code
rg -A 5 "json.Unmarshal.*ofac"
Length of output: 92
Script:
#!/bin/bash
# Search for functions that handle the OFAC list
rg -A 10 "DefaultOfacListURL|OfacListFilename" client/chain/ofac.go
# Look for JSON unmarshaling or loading functions
ast-grep --pattern 'func $_($$$) {
$$$
json.$$$
$$$
}' client/chain/ofac.go
Length of output: 1005
Script:
#!/bin/bash
# Get the full content of the OFAC checker implementation
cat client/chain/ofac.go
Length of output: 1916
PavelInjective
force-pushed
the
fix/update-ofac-name
branch
from
9cf50f3 to
c9701fa
Compare
PavelInjective
force-pushed
the
fix/update-ofac-name
branch
from
c9701fa to
9bc78b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
client/chain/ofac.go (1)
Line range hint
157-160: Verify Ethereum address checksumsThe newly added addresses have inconsistent checksum formats:
0x0992E2D17e0082Df8a31Bf36Bd8Cc662551de68Buses mixed case (checksum format)- Others use lowercase
Consider normalizing all addresses to either:
- Lowercase (preferred for consistency with existing addresses)
- EIP-55 checksum format (more secure but requires changes to all addresses)
Apply this diff to normalize to lowercase:
"0xffbac21a641dcfe4552920138d90f3638b3c9fba", "0xc5801cd781d168e2d3899ad9c39d8a2541871298", - "0x0992E2D17e0082Df8a31Bf36Bd8Cc662551de68B", + "0x0992e2d17e0082df8a31bf36bd8cc662551de68b", "0x8aa07899eb940f40e514b8efdb3b6af5d1cf7bb"injective_data/ofac.json (2)
Line range hint
1-160: Warning: Duplicate OFAC list files detected.The same OFAC list appears to exist in two locations:
injective_data/ofac.jsonclient/chain/injective_data/ofac.jsonThis duplication could lead to maintenance issues and inconsistencies. Consider:
- Keeping only one canonical version
- Using symbolic links if multiple locations are required
- Adding documentation to explain the reason for duplication if it's intentional
Invalid Ethereum address format detected and potential compliance issues
The verification revealed several issues that need immediate attention:
- The address
0x8aa07899eb940f40e514b8efdb3b6af5d1cf7bbis invalid as it's only 39 characters long (should be 40 characters after '0x')- The changes show one address was moved/reordered (
0xffbac21a641dcfe4552920138d90f3638b3c9fba) and two new addresses were added- All addresses are unique (no duplicates found)
Action items:
- Fix the malformed address
0x8aa07899eb940f40e514b8efdb3b6af5d1cf7bb- Ensure the new additions (
0xc5801cd781d168e2d3899ad9c39d8a2541871298,0x0992E2D17e0082Df8a31Bf36Bd8Cc662551de68B) are from official OFAC updates🔗 Analysis chain
Line range hint
1-160: Ensure compliance with OFAC requirements.The changes involve:
- Addition of multiple new addresses
- Removal of some existing addresses
- Reordering of addresses
Please verify:
- All changes are based on official OFAC updates
- No required addresses were accidentally removed
- The changes have been reviewed by the compliance team
Let me analyze the changes more thoroughly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare with previous version to identify all changes # Test: List removed addresses git diff HEAD^ HEAD -- injective_data/ofac.json | grep '^-' | grep -oE '0x[0-9a-fA-F]{40}' # Test: List added addresses git diff HEAD^ HEAD -- injective_data/ofac.json | grep '^+' | grep -oE '0x[0-9a-fA-F]{40}'Length of output: 361
Script:
#!/bin/bash # Get the full list of addresses from the current version cat injective_data/ofac.json | jq -r '.[]' | sort > /tmp/current_addresses.txt # Count total addresses echo "Total addresses in current version:" wc -l /tmp/current_addresses.txt # Check for duplicate addresses (case-insensitive) echo -e "\nChecking for duplicates (case-insensitive):" cat /tmp/current_addresses.txt | tr '[:upper:]' '[:lower:]' | sort | uniq -d # Verify address format echo -e "\nAddresses with invalid format:" cat /tmp/current_addresses.txt | grep -v '^0x[0-9a-fA-F]\{40\}$' || trueLength of output: 566
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
client/chain/injective_data/ofac.json(1 hunks)client/chain/ofac.go(1 hunks)injective_data/ofac.json(1 hunks)
🔇 Additional comments (2)
client/chain/ofac.go (1)
Line range hint 2-160: Remove duplicate OFAC list file
There are two identical OFAC list files:
injective_data/ofac.jsonclient/chain/injective_data/ofac.json
Having duplicate files could lead to synchronization issues.
Please:
- Determine which file is the source of truth
- Remove the duplicate file
- Update all references to use the correct path
✅ Verification successful
Let me gather more information to verify if these files actually exist and how they're being used.
Let me check how these files are referenced in the code to determine which one is actually being used.
Remove duplicate OFAC list file from client/chain/injective_data/
The code in client/chain/ofac.go uses the path defined by OfacListPath = "injective_data", which points to the root-level injective_data/ofac.json. The duplicate file in client/chain/injective_data/ofac.json is not referenced anywhere in the code and should be removed.
Please:
- Remove the duplicate file at
client/chain/injective_data/ofac.json - Remove the
client/chain/injective_datadirectory if it becomes empty
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and usage of both OFAC list files
# Check for references to both paths
echo "Searching for references to injective_data/ofac.json:"
rg "injective_data/ofac\.json"
echo "Searching for references to client/chain/injective_data/ofac.json:"
rg "client/chain/injective_data/ofac\.json"
# Compare the files if both exist
echo "Comparing the files:"
diff injective_data/ofac.json client/chain/injective_data/ofac.json || true
Length of output: 459
Script:
#!/bin/bash
# Check if the directories and files exist
echo "Checking directories and files:"
ls -la injective_data/ 2>/dev/null || echo "injective_data/ does not exist"
ls -la client/chain/injective_data/ 2>/dev/null || echo "client/chain/injective_data/ does not exist"
# Search for any reference to ofac.json or OfacListPath
echo -e "\nSearching for references to ofac.json or OfacListPath:"
rg -l "ofac\.json|OfacListPath"
# Search for any file named ofac.json
echo -e "\nSearching for any file named ofac.json:"
fd "ofac.json"
Length of output: 857
Script:
#!/bin/bash
# Check the content of ofac.go and ofac_test.go to see how the paths are used
echo "Content of client/chain/ofac.go:"
cat client/chain/ofac.go
echo -e "\nContent of client/chain/ofac_test.go:"
cat client/chain/ofac_test.go
Length of output: 4214
client/chain/injective_data/ofac.json (1)
1-160: Skipping review of duplicate file.
This file is identical to injective_data/ofac.json which has already been reviewed.
| @@ -10,7 +10,7 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| DefaultOfacListURL = "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofac.json" | |||
| DefaultOfacListURL = "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofacAndRestricted.json" | |||
There was a problem hiding this comment.
💡 Codebase verification
Update local filename to match remote OFAC list source
The verification confirms that there is an inconsistency between the remote source (ofacAndRestricted.json) and local storage (ofac.json). The code downloads from DefaultOfacListURL and saves locally using OfacListFilename. This mismatch should be fixed to maintain consistency and prevent confusion.
client/chain/ofac.go: UpdateOfacListFilenamefromofac.jsontoofacAndRestricted.jsonto match the source URL
var (
OfacListPath = "injective_data"
- OfacListFilename = "ofac.json"
+ OfacListFilename = "ofacAndRestricted.json"
)🔗 Analysis chain
Verify consistency between URL and local filename
The URL has been updated to fetch "ofacAndRestricted.json", but the local OfacListFilename variable still uses "ofac.json". This inconsistency could lead to confusion and potential issues.
Apply this diff to maintain consistency:
var (
OfacListPath = "injective_data"
- OfacListFilename = "ofac.json"
+ OfacListFilename = "ofacAndRestricted.json"
)Let me gather more information about how this filename is used in the codebase to understand the potential impact.
Let me check how the OFAC list is actually loaded and used to understand if this filename mismatch could cause issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of OFAC filenames across the codebase
# Search for references to ofac.json and ofacAndRestricted.json
echo "Searching for OFAC filename references:"
rg -i "ofac.*\.json"
# Search for URL references
echo "Searching for OFAC URL references:"
rg "https.*injective-lists.*json"
Length of output: 1226
Script:
#!/bin/bash
# Search for functions/methods that use OfacListFilename
ast-grep --pattern 'OfacListFilename'
# Search for any file operations or references to these filenames
rg -A 3 'ofac.*\.json'
# Look for any configuration or setup code related to OFAC
ast-grep --pattern 'func $_($$_) {
$$$
OfacList$_
$$$
}'
Length of output: 640
Script:
#!/bin/bash
# Look for methods that load or use the OFAC list
ast-grep --pattern 'func ($_ *OfacChecker) $_($$$) {
$$$
}'
# Search for any file operations related to loading OFAC data
rg -A 5 'LoadOfacList|SaveOfacList'
# Check if there's any download or update functionality
rg -A 5 'DefaultOfacListURL'
Length of output: 707
| "0xffbac21a641dcfe4552920138d90f3638b3c9fba", | ||
| "0xc5801cd781d168e2d3899ad9c39d8a2541871298", | ||
| "0x0992E2D17e0082Df8a31Bf36Bd8Cc662551de68B", | ||
| "0x8aa07899eb940f40e514b8efdb3b6af5d1cf7bb" |
There was a problem hiding this comment.
💡 Codebase verification
Invalid Ethereum address format detected
The address 0x8aa07899eb940f40e514b8efdb3b6af5d1cf7bb is invalid as it contains only 39 hexadecimal characters after the '0x' prefix, instead of the required 40 characters for a valid Ethereum address.
- The address appears to be missing one character at the end
- Other addresses in the list are properly formatted with 40 hexadecimal characters
- This invalid address is present in both
injective_data/ofac.jsonandclient/chain/injective_data/ofac.json
🔗 Analysis chain
Verify the newly added addresses.
The following addresses have been added:
- 0xc5801cd781d168e2d3899ad9c39d8a2541871298
- 0x0992E2D17e0082Df8a31Bf36Bd8Cc662551de68B
- 0x8aa07899eb940f40e514b8efdb3b6af5d1cf7bb
Please ensure these addresses are from official OFAC sources.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the format of the newly added Ethereum addresses
# Test: Check if all addresses are valid Ethereum addresses (40 hex chars)
for addr in "0xc5801cd781d168e2d3899ad9c39d8a2541871298" "0x0992E2D17e0082Df8a31Bf36Bd8Cc662551de68B" "0x8aa07899eb940f40e514b8efdb3b6af5d1cf7bb"; do
if ! echo "$addr" | grep -qE "^0x[0-9a-fA-F]{40}$"; then
echo "Invalid address format: $addr"
fi
done
# Test: Check if addresses are unique in the file
rg -c "$addr" injective_data/ofac.json client/chain/injective_data/ofac.json
Length of output: 1012
There was a problem hiding this comment.
@PavelInjective I think we should remove the file "client/chain/injective_data/ofac.json" since the file being used is the one in "injective_data/ofac.json"
Also please update the PR description to include the Jira task link
Solves issue CHAIN 253
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes