(fix) Update OFAC list URL and filename #362
Conversation
WalkthroughThe pull request updates two constants in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 1
🧹 Outside diff range and nitpick comments (2)
pyinjective/ofac.py (2)
Line range hint
22-24: Update error message to reflect new filename.The error message still uses the generic term "OFAC list" while we're now dealing with both OFAC and restricted lists.
- f"Error parsing OFAC list. Please, download it by running python3 pyinjective/ofac_list.py ({e})" + f"Error parsing OFAC and restricted list. Please, download it by running python3 pyinjective/ofac_list.py ({e})"
8-10: Security Impact Assessment RequiredThis change expands the scope of blocked addresses by combining OFAC and other restricted lists. Please ensure:
- The expanded list is properly validated and maintained
- All stakeholders are aware of the expanded blocking scope
- Compliance requirements are met with the new combined list
Consider adding:
- List version tracking
- Validation of list format and content during download
- Audit logging for list updates
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
pyinjective/ofac.py(1 hunks)
🔇 Additional comments (1)
pyinjective/ofac.py (1)
8-8: Verify the new OFAC list endpoint and its implications.
The URL change from ofac.json to ofacAndRestricted.json suggests an expansion in the scope of blocked addresses. This could potentially affect existing integrations.
Let's verify the endpoint and its content:
✅ Verification successful
URL change to ofacAndRestricted.json is safe to proceed
The verification shows:
- Both endpoints are accessible and maintain the same array structure
- The new list contains 159 addresses (slight increase from 156)
- Usage in the codebase is consistent:
composer.pyonly instantiates the checkerbroadcaster.pyuses it for address validation
- No breaking changes in implementation as the data structure remains compatible
The expanded scope with additional restricted addresses enhances security coverage while maintaining backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new OFAC list endpoint and compare with old endpoint
# Check if the new endpoint exists and is accessible
curl -s -f -I "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofacAndRestricted.json" || echo "New endpoint not accessible"
# Compare with old endpoint to understand scope changes
curl -s "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofac.json" | jq 'length' || echo "Old endpoint not accessible"
curl -s "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofacAndRestricted.json" | jq 'length' || echo "New endpoint not accessible"
Length of output: 1283
Script:
#!/bin/bash
# Let's check how the OfacChecker class uses this URL and if there are any dependencies
# First, let's see the full implementation of OfacChecker
ast-grep --pattern 'class OfacChecker {
$$$
}'
# Check for any other files that might be importing or using OfacChecker
rg "from.*ofac.*import.*OfacChecker|import.*ofac.*OfacChecker" -l
# Check if there are any tests that might need updating
fd "test.*ofac.*\.py$"
Length of output: 199
Script:
#!/bin/bash
# Let's check how composer.py and broadcaster.py use OfacChecker
# Check the usage in composer.py
rg -A 5 "OfacChecker" pyinjective/composer.py
# Check the usage in broadcaster.py
rg -A 5 "OfacChecker" pyinjective/core/broadcaster.py
# Let's also verify if the new JSON structure is compatible
# by checking a sample from both endpoints
curl -s "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofac.json" | jq 'type'
curl -s "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofacAndRestricted.json" | jq 'type'
Length of output: 1404
pyinjective/ofac.py
Outdated
| ) | ||
| OFAC_LIST_FILENAME_DEFAULT = "ofac.json" | ||
| OFAC_LIST_FILENAME_DEFAULT = "ofacAndRestricted.json" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling migration from old to new filename.
The filename change might affect existing installations that have cached the old ofac.json file. Consider:
- Adding migration logic to handle existing installations
- Adding a warning log when old file exists
Here's a suggested implementation:
OFAC_LIST_FILENAME_DEFAULT = "ofacAndRestricted.json"
OFAC_LIST_FILENAME = OFAC_LIST_FILENAME_DEFAULT
+OLD_OFAC_LIST_FILENAME = "ofac.json" # For migration
class OfacChecker:
def __init__(self):
self._ofac_list_path = self.get_ofac_list_path()
+ old_path = os.path.join(os.path.dirname(__file__), OLD_OFAC_LIST_FILENAME)
+ if os.path.exists(old_path):
+ import warnings
+ warnings.warn(f"Found old OFAC list file {OLD_OFAC_LIST_FILENAME}. Please remove it as it's no longer used.")
try:
with open(self._ofac_list_path, "r") as f:Committable suggestion skipped: line range outside the PR's diff.
PavelInjective
force-pushed
the
fix/ofac-filename
branch
3 times, most recently
from
3ea588e to
aee17b4
Compare
PavelInjective
force-pushed
the
fix/ofac-filename
branch
from
aee17b4 to
3c1df6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
6-8: Enhance the changelog entry with more details.The current changelog entry could be more informative. Consider expanding it to include:
- The specific URL changes (from/to)
- The nature of content updates
- Any potential impact on existing functionality
- Migration steps if needed
This additional context would help users better understand the scope and impact of these security-related changes.
Example enhancement:
## [1.8.1] - 2024-12-04 ### Changed -Updated OFAC list link and contents +- Updated OFAC list URL from `ofac.json` to `ofacAndRestricted.json` +- Updated OFAC list contents to include latest restricted addresses +- No changes to existing functionality - automatic migration for existing users
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)pyinjective/ofac.json(1 hunks)pyinjective/ofac.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyinjective/ofac.json
🚧 Files skipped from review as they are similar to previous changes (1)
- pyinjective/ofac.py
PavelInjective
force-pushed
the
fix/ofac-filename
branch
from
3c1df6b to
9395b55
Compare
|
@PavelInjective please update the PR description to point to the Jira task |
Solves issue 252.
Summary by CodeRabbit
New Features
Bug Fixes