Release/v1.7.0 #347
Release/v1.7.0 #347
Conversation
WalkthroughThe recent update to the project introduces OFAC (Office of Foreign Assets Control) address validations, enhancing compliance by checking restricted addresses before processing transactions. Key changes include the addition of the Changes
Possibly related PRs
Suggested reviewers
Poem
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (1)
Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (10)
- CHANGELOG.md (1 hunks)
- pyinjective/composer.py (4 hunks)
- pyinjective/core/broadcaster.py (2 hunks)
- pyinjective/denoms_devnet.ini (3 hunks)
- pyinjective/ofac.json (1 hunks)
- pyinjective/ofac.py (1 hunks)
- pyinjective/proto/google/api/client_pb2.py (3 hunks)
- pyproject.toml (1 hunks)
- tests/core/test_broadcaster.py (1 hunks)
- tests/test_composer_deprecation_warnings.py (8 hunks)
Files skipped from review due to trivial changes (2)
- pyproject.toml
- tests/test_composer_deprecation_warnings.py
Additional context used
Ruff
tests/core/test_broadcaster.py
41-41:
pytest.raises(Exception)should be considered evil(B017)
pyinjective/ofac.py
44-44: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
LanguageTool
CHANGELOG.md
[duplication] ~6-~6: Possible typo: you repeated a word
Context: ...this file. ## [1.7.0] - 2024-09-18 ### Added - Added OFAC restricted addresses validations ...(ENGLISH_WORD_REPEAT_RULE)
Additional comments not posted (23)
tests/core/test_broadcaster.py (3)
17-26: LGTM!The
setUpmethod correctly sets up the test environment by creating a temporary OFAC list file containing a banned address derived from a private key. The use of thewithstatement ensures that the file is closed after writing.
28-30: LGTM!The
tearDownmethod correctly cleans up the test environment by removing the temporary OFAC list file and resetting theOFAC_LIST_FILENAMEto its default value.
32-48: LGTM!The
test_broadcast_address_in_ofac_listmethod correctly tests the behavior of theMsgBroadcasterWithPkclass when attempting to broadcast a transaction from an address that is listed in the OFAC sanctions list. The use of thepytest.raisescontext manager to assert that an exception is raised is a good practice for testing expected exceptions.Regarding the static analysis hint from Ruff:
The use of
pytest.raises(Exception)is appropriate in this case for testing the expected exception. This is a false positive from Ruff and can be safely ignored.Tools
Ruff
41-41:
pytest.raises(Exception)should be considered evil(B017)
pyinjective/ofac.py (2)
12-47: LGTM!The
OfacCheckerclass is well-structured and follows the Single Responsibility Principle. The initialization logic ensures that the OFAC list is available before proceeding, and loading the blacklist into a set is an efficient approach for fast lookups.Tools
Ruff
44-44: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
13-21: LGTM!The constructor correctly checks for the existence of the OFAC list file and raises an informative exception if it is missing. Loading the blacklist into a set is an efficient approach for fast lookups.
pyinjective/ofac.json (2)
1-48: The JSON structure is valid and aligns with the PR objective.The file follows a valid JSON format, with each address represented as a string within an array. This structure facilitates easy parsing and integration into the system for OFAC compliance validations.
1-48: Verify the completeness and accuracy of the address list.Given the critical nature of OFAC compliance, it is crucial to ensure that the address list is comprehensive and up to date.
Please confirm that the list has been sourced from a reliable and authoritative data provider and that there is a process in place to regularly update the list as new sanctions are issued.
pyinjective/denoms_devnet.ini (4)
Line range hint
11-19: LGTM!The new entry for the derivative trading pair INJ/USDT PERP follows the correct format and includes reasonable parameter values.
29-31: Looks good!The new entry for the ATOM token is properly formatted with a valid IBC address for the peggy denomination and a reasonable number of decimals.
45-47: Verify the impact of the peggy denomination change.The update to the DAI token's peggy denomination to a different Ethereum address looks good. However, please ensure that this change corresponds to a planned token migration or contract upgrade, and that it is properly communicated to users.
To verify the impact of the peggy denomination change, run the following script:
Verification successful
Peggy denomination change verified for devnet environment
The update to the DAI token's peggy denomination has been verified. The change is specific to the devnet environment (pyinjective/denoms_devnet.ini) and does not affect the mainnet or testnet configurations. This suggests an intentional change for testing or preparation for a future update.
To ensure a smooth transition:
- Confirm that this change is intended only for the devnet environment.
- Update any devnet-specific documentation or guides to reflect this change.
- Plan for eventual propagation of this change to testnet and mainnet, if applicable.
- Communicate the change to developers working with the devnet environment.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the old and new peggy denominations for DAI token. # Test 1: Search for usage of the old peggy denomination. Expect: No occurrences. rg --type python $'peggy0x6B175474E89094C44Da98b954EedeAC495271d0F' # Test 2: Search for usage of the new peggy denomination. Expect: Occurrences in relevant code. rg --type python $'peggy0x6b175474e89094c44da98b954eedeac495271d0f'Length of output: 195
Script:
#!/bin/bash # Description: List file extensions and search for peggy denominations # List all file extensions in the repository echo "File extensions in the repository:" fd -t f -x basename {} | grep -o '\.[^.]*$' | sort | uniq -c | sort -rn # Search for the old peggy denomination echo -e "\nSearching for old peggy denomination:" rg 'peggy0x6B175474E89094C44Da98b954EedeAC495271d0F' # Search for the new peggy denomination echo -e "\nSearching for new peggy denomination:" rg 'peggy0x6b175474e89094c44da98b954eedeac495271d0f'Length of output: 863
41-43: Verify the impact of the peggy denomination change.The update to the CHZ token's peggy denomination from a legacy address to a factory address looks good. However, please ensure that this change is properly communicated to users and any necessary migrations are handled smoothly.
To verify the impact of the peggy denomination change, run the following script:
pyinjective/proto/google/api/client_pb2.py (2)
54-57: LGTM!The addition of the
PythonSettingsmessage with theExperimentalFeaturessub-message and therest_async_io_enabledfield is a valid change to introduce a new experimental feature in the Python client library.
36-39: Looks good!The adjustments to the serialized positions of the
_CLIENTLIBRARYORGANIZATIONand_CLIENTLIBRARYDESTINATIONenums are necessary to accommodate the new fields added in thePythonSettingsmessage.pyinjective/core/broadcaster.py (3)
15-15: LGTM!The import statement is correct and aligns with the PR objective of adding OFAC restricted address validations.
66-66: LGTM!The
OfacCheckerinstance is correctly initialized and assigned to theself._ofac_checkerattribute.
68-69: LGTM!The OFAC blacklist check is implemented correctly:
- The
is_blacklistedmethod is called with thetrading_injective_addressas an argument.- If the address is blacklisted, an exception is raised with a clear error message.
- This check prevents transactions from blacklisted addresses, ensuring compliance with regulatory requirements.
The check introduces a critical control flow alteration in the constructor, effectively blocking any further operations with blacklisted addresses.
CHANGELOG.md (1)
5-7: LGTM!The changelog entry for version 1.7.0 accurately captures the key enhancement of adding OFAC restricted addresses validations. This change demonstrates a commitment to regulatory compliance and risk management.
Tools
LanguageTool
[duplication] ~6-~6: Possible typo: you repeated a word
Context: ...this file. ## [1.7.0] - 2024-09-18 ### Added - Added OFAC restricted addresses validations ...(ENGLISH_WORD_REPEAT_RULE)
pyinjective/composer.py (6)
151-151: LGTM!The addition of the
OfacCheckerinstance in the constructor looks good. It will allow performing checks against the OFAC blacklist.
476-477: Great addition of the OFAC compliance check!The OFAC check on the granter address is a crucial compliance requirement. Raising an exception if the granter is found in the OFAC list prevents unauthorized access.
2132-2133: OFAC compliance check looks good!The addition of the OFAC check on the granter address in
MsgGrantTypedensures compliance and prevents unauthorized access by blacklisted addresses.
Line range hint
2400-2450: Skipping review.The
MsgResponsesmethod remains unchanged and does not require any modifications related to the OFAC compliance changes.
Line range hint
2452-2462: Skipping review.The
UnpackMsgExecResponsemethod remains unchanged and does not require any modifications related to the OFAC compliance changes.
Line range hint
2464-2476: Skipping review.The
UnpackTransactionMessagesmethod remains unchanged and does not require any modifications related to the OFAC compliance changes.
| async def download_ofac_list(cls): | ||
| async with aiohttp.ClientSession() as session: | ||
| try: | ||
| async with session.get(OFAC_LIST_URL) as response: | ||
| response.raise_for_status() | ||
| text_content = await response.text() | ||
| ofac_list = json.loads(text_content) | ||
| ofac_file_path = cls.get_ofac_list_path() | ||
| with open(ofac_file_path, "w") as f: | ||
| json.dump(ofac_list, f, indent=2) | ||
| f.write("\n") | ||
| return | ||
| except Exception as e: | ||
| raise Exception(f"Error fetching OFAC list: {e}") |
There was a problem hiding this comment.
Improve exception handling.
The function correctly uses aiohttp to asynchronously fetch the OFAC list from the specified URL and handles exceptions, raising an informative exception if an error occurs during the download process.
However, to distinguish exceptions raised within the except clause from errors in exception handling, consider using raise ... from err or raise ... from None.
Apply this diff to improve exception handling:
- except Exception as e:
- raise Exception(f"Error fetching OFAC list: {e}")
+ except Exception as e:
+ raise Exception(f"Error fetching OFAC list: {e}") from eCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def download_ofac_list(cls): | |
| async with aiohttp.ClientSession() as session: | |
| try: | |
| async with session.get(OFAC_LIST_URL) as response: | |
| response.raise_for_status() | |
| text_content = await response.text() | |
| ofac_list = json.loads(text_content) | |
| ofac_file_path = cls.get_ofac_list_path() | |
| with open(ofac_file_path, "w") as f: | |
| json.dump(ofac_list, f, indent=2) | |
| f.write("\n") | |
| return | |
| except Exception as e: | |
| raise Exception(f"Error fetching OFAC list: {e}") | |
| async def download_ofac_list(cls): | |
| async with aiohttp.ClientSession() as session: | |
| try: | |
| async with session.get(OFAC_LIST_URL) as response: | |
| response.raise_for_status() | |
| text_content = await response.text() | |
| ofac_list = json.loads(text_content) | |
| ofac_file_path = cls.get_ofac_list_path() | |
| with open(ofac_file_path, "w") as f: | |
| json.dump(ofac_list, f, indent=2) | |
| f.write("\n") | |
| return | |
| except Exception as e: | |
| raise Exception(f"Error fetching OFAC list: {e}") from e |
Tools
Ruff
44-44: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
| [ | ||
| "0x179f48c78f57a3a78f0608cc9197b8972921d1d2", | ||
| "0x1967d8af5bd86a497fb3dd7899a020e47560daaf", | ||
| "0x19aa5fe80d33a56d56c78e82ea5e50e5d80b4dff", | ||
| "0x19aa5fe80d33a56d56c78e82ea5e50e5d80b4dff", | ||
| "0x1da5821544e25c636c1417ba96ade4cf6d2f9b5a", | ||
| "0x2f389ce8bd8ff92de3402ffce4691d17fc4f6535", | ||
| "0x2f389ce8bd8ff92de3402ffce4691d17fc4f6535", | ||
| "0x2f50508a8a3d323b91336fa3ea6ae50e55f32185", | ||
| "0x308ed4b7b49797e1a98d3818bff6fe5385410370", | ||
| "0x3cbded43efdaf0fc77b9c55f6fc9988fcc9b757d", | ||
| "0x3efa30704d2b8bbac821307230376556cf8cc39e", | ||
| "0x48549a34ae37b12f6a30566245176994e17c6b4a", | ||
| "0x4f47bc496083c727c5fbe3ce9cdf2b0f6496270c", | ||
| "0x4f47bc496083c727c5fbe3ce9cdf2b0f6496270c", | ||
| "0x4f47bc496083c727c5fbe3ce9cdf2b0f6496270c", | ||
| "0x530a64c0ce595026a4a556b703644228179e2d57", | ||
| "0x5512d943ed1f7c8a43f3435c85f7ab68b30121b0", | ||
| "0x5a7a51bfb49f190e5a6060a5bc6052ac14a3b59f", | ||
| "0x5f48c2a71b2cc96e3f0ccae4e39318ff0dc375b2", | ||
| "0x6be0ae71e6c41f2f9d0d1a3b8d0f75e6f6a0b46e", | ||
| "0x6f1ca141a28907f78ebaa64fb83a9088b02a8352", | ||
| "0x746aebc06d2ae31b71ac51429a19d54e797878e9", | ||
| "0x77777feddddffc19ff86db637967013e6c6a116c", | ||
| "0x797d7ae72ebddcdea2a346c1834e04d1f8df102b", | ||
| "0x8576acc5c05d6ce88f4e49bf65bdf0c62f91353c", | ||
| "0x901bb9583b24d97e995513c6778dc6888ab6870e", | ||
| "0x961c5be54a2ffc17cf4cb021d863c42dacd47fc1", | ||
| "0x97b1043abd9e6fc31681635166d430a458d14f9c", | ||
| "0x9c2bc757b66f24d60f016b6237f8cdd414a879fa", | ||
| "0x9f4cda013e354b8fc285bf4b9a60460cee7f7ea9", | ||
| "0xa7e5d5a720f06526557c513402f2e6b5fa20b008", | ||
| "0xb6f5ec1a0a9cd1526536d3f0426c429529471f40", | ||
| "0xb6f5ec1a0a9cd1526536d3f0426c429529471f40", | ||
| "0xb6f5ec1a0a9cd1526536d3f0426c429529471f40", | ||
| "0xc455f7fd3e0e12afd51fba5c106909934d8a0e4a", | ||
| "0xca0840578f57fe71599d29375e16783424023357", | ||
| "0xd0975b32cea532eadddfc9c60481976e39db3472", | ||
| "0xd882cfc20f52f2599d84b8e8d58c7fb62cfe344b", | ||
| "0xd882cfc20f52f2599d84b8e8d58c7fb62cfe344b", | ||
| "0xe1d865c3d669dcc8c57c8d023140cb204e672ee4", | ||
| "0xe7aa314c77f4233c18c6cc84384a9247c0cf367b", | ||
| "0xed6e0a7e4ac94d976eebfb82ccf777a3c6bad921", | ||
| "0xf3701f445b6bdafedbca97d1e477357839e4120d", | ||
| "0xfac583c0cf07ea434052c49115a4682172ab6b4f", | ||
| "0xfec8a60023265364d066a1212fde3930f6ae8da7", | ||
| "0xffbac21a641dcfe4552920138d90f3638b3c9fba" | ||
| ] |
There was a problem hiding this comment.
Consider removing duplicate entries.
The presence of duplicate addresses (e.g., "0x19aa5fe80d33a56d56c78e82ea5e50e5d80b4dff" on lines 4-5) suggests potential issues with the data compilation process. Duplicate entries may lead to inefficiencies or confusion in the validation process.
Consider reviewing the data source and deduplicating the array to ensure data integrity and optimize performance.
pyinjective/denoms_devnet.ini
Outdated
| peggy_denom = factory/inj1uukt3kqela4vsllvrqnrgllkna5wn3cm588w6k/inj1kwdranvwf6vl2grh99layugwdnph6um2e8k25g | ||
| decimals = 0 | ||
|
|
There was a problem hiding this comment.
Please clarify the purpose of the unknown token and its unusual configuration.
The new entry for an unknown token with a factory address as the peggy denomination and 0 decimals raises some concerns:
- What does this unknown token represent? Is it a placeholder for a future token?
- Using a factory address for an unknown token seems unusual. Is there a specific reason for this choice?
- Setting the number of decimals to 0 may cause issues with precision. Is this intentional?
Please provide more context on this token and consider adjusting its configuration if necessary.
pyinjective/denoms_devnet.ini
Outdated
| [DOT] | ||
| peggy_denom = ibc/624BA9DD171915A2B9EA70F69638B2CEA179959850C1A586F6C485498F29EDD4 | ||
| decimals = 10 |
There was a problem hiding this comment.
Tip
Codebase Verification
Review needed: Inconsistent peggy denominations for DOT token
The new IBC address for the DOT token's peggy denomination has been consistently applied across devnet, mainnet, and testnet configurations in the respective .ini files. However, an important issue has been identified:
- The old peggy denomination (peggy0x6B175474E89094C44Da98b954EedeAC495271d0F) is still present in the same files alongside the new IBC address.
This coexistence of old and new denominations could lead to confusion or potential errors. Please review the migration process and determine if the old denomination should be removed or if there's a specific reason for maintaining both (e.g., backward compatibility).
Affected files:
- pyinjective/denoms_devnet.ini
- pyinjective/denoms_mainnet.ini
- pyinjective/denoms_testnet.ini
Analysis chain
Verify the impact of the peggy denomination change.
The update to the DOT token's peggy denomination to an IBC address looks good. This change suggests a migration from a native peggy token to an IBC token. Please ensure that this migration is properly executed and communicated to users.
To verify the impact of the peggy denomination change, run the following script:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the old and new peggy denominations for DOT token.
# Test 1: Search for usage of the old peggy denomination. Expect: No occurrences.
rg --type python $'peggy0x6B175474E89094C44Da98b954EedeAC495271d0F'
# Test 2: Search for usage of the new peggy denomination. Expect: Occurrences in relevant code.
rg --type python $'ibc/624BA9DD171915A2B9EA70F69638B2CEA179959850C1A586F6C485498F29EDD4'
Length of output: 216
Script:
#!/bin/bash
# Description: Verify the usage of the old and new peggy denominations for DOT token using a more generic approach.
# Find Python files and search for the old peggy denomination
echo "Searching for old peggy denomination in Python files:"
fd -e py -x rg -i 'peggy0x6B175474E89094C44Da98b954EedeAC495271d0F' {}
echo "---"
# Find Python files and search for the new peggy denomination
echo "Searching for new peggy denomination in Python files:"
fd -e py -x rg -i 'ibc/624BA9DD171915A2B9EA70F69638B2CEA179959850C1A586F6C485498F29EDD4' {}
echo "---"
# Perform a repository-wide search for the old peggy denomination
echo "Repository-wide search for old peggy denomination:"
rg -i 'peggy0x6B175474E89094C44Da98b954EedeAC495271d0F'
echo "---"
# Perform a repository-wide search for the new peggy denomination
echo "Repository-wide search for new peggy denomination:"
rg -i 'ibc/624BA9DD171915A2B9EA70F69638B2CEA179959850C1A586F6C485498F29EDD4'
Length of output: 1419
pyinjective/denoms_devnet.ini
Outdated
There was a problem hiding this comment.
Why were the denoms for devnet removed?
Created this PR to release all the latest changes in
devbranch tomasterfor version 1.7.0Summary by CodeRabbit
Release Notes for Version 1.7.0
New Features
Bug Fixes
Configuration Changes
Testing Enhancements