Fix/make OFAC list file path relative #251
Conversation
…ps to be able to read/download the file
…ved the test to use a temp test directory
WalkthroughThe pull request involves several changes across multiple files related to the Changes
Possibly related PRs
Suggested reviewers
🪧 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: 2
🧹 Outside diff range and nitpick comments (8)
client/chain/ofac.go (4)
13-13: LGTM! Consider adding a version to the URL.The constant
DefaultOfacListURLis correctly declared and follows Go naming conventions. However, consider adding a version or commit hash to the URL to ensure consistency and avoid potential issues if the repository structure changes.Example:
DefaultOfacListURL = "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/v1.0.0/wallets/ofac.json"
17-18: LGTM! Consider environment-based configuration.The variables
OfacListPathandOfacListFilenameare correctly declared and follow Go naming conventions. However, to improve flexibility across different environments, consider allowing these values to be configurable through environment variables or a configuration file.Example:
var ( OfacListPath = getEnv("OFAC_LIST_PATH", "injective_data") OfacListFilename = getEnv("OFAC_LIST_FILENAME", "ofac.json") ) func getEnv(key, fallback string) string { if value, ok := os.LookupEnv(key); ok { return value } return fallback }
41-43: LGTM! Consider adding error handling for empty paths.The
GetOfacListPathfunction is well-implemented and follows Go naming conventions. Usingpath.Joinis the correct approach for constructing file paths. To improve robustness, consider adding a check for emptyOfacListPathorOfacListFilename.Example:
func GetOfacListPath() (string, error) { if OfacListPath == "" || OfacListFilename == "" { return "", fmt.Errorf("OfacListPath or OfacListFilename is empty") } return path.Join(OfacListPath, OfacListFilename), nil }
56-59: LGTM! Consider handling potential error from GetOfacListPath.The changes to
DownloadOfacListare good improvements. Creating the directory before writing the file ensures the path exists, and usingGetOfacListPath()centralizes the path logic. However, ifGetOfacListPath()is modified to return an error as suggested earlier, make sure to handle that potential error here.Example:
path, err := GetOfacListPath() if err != nil { return err } outFile, err := os.Create(path) if err != nil { return err }injective_data/ofac.json (1)
1-48: JSON structure is valid, but consider sorting and deduplicating the list.The JSON file is well-formed and contains a valid array of Ethereum addresses. However, there are a few improvements that could be made:
- The list is not sorted, which may impact performance for large lists when searching for specific addresses.
- There are duplicate entries in the list, which is unnecessary and increases the file size.
Consider applying the following improvements:
- Sort the list of addresses alphabetically for easier maintenance and potentially faster lookups.
- Remove duplicate entries to reduce file size and improve clarity.
Here's a script to help you sort and deduplicate the list:
#!/bin/bash # Sort and deduplicate the OFAC list jq 'unique | sort' injective_data/ofac.json > injective_data/ofac_sorted_unique.json && mv injective_data/ofac_sorted_unique.json injective_data/ofac.jsonThis script will create a new file with sorted and unique addresses, then replace the original file.
client/chain/ofac_test.go (2)
26-53: LGTM: SetupTest method is comprehensive, with a minor suggestion.The
SetupTestmethod effectively initializes the test environment, including network setup and OFAC list preparation. It properly handles errors and sets up a temporary OFAC list for testing purposes.Consider using
deferfor closing the file to ensure it's always closed, even if an error occurs:file, err := os.Create(chain.GetOfacListPath()) suite.NoError(err) +defer file.Close() _, err = io.WriteString(file, string(jsonData)) suite.NoError(err) -err = file.Close() -suite.NoError(err)
59-83: LGTM: Comprehensive OFAC testing, with a suggestion for expansion.The
TestOfacListmethod effectively tests the OFAC checker's functionality, including both positive and negative cases. It also verifies the expected error when creating a new chain client with a blacklisted address. TheTestOfacTestSuitefunction correctly runs the entire test suite.Consider adding more test cases to increase coverage:
- Test with multiple addresses in the OFAC list.
- Test with an empty OFAC list.
- Test the behavior when the OFAC list file is missing or corrupted.
Example additional test case:
func (suite *OfacTestSuite) TestEmptyOfacList() { // Create an empty OFAC list emptyList := []string{} jsonData, err := json.Marshal(emptyList) suite.NoError(err) // Write empty list to file err = os.WriteFile(chain.GetOfacListPath(), jsonData, 0644) suite.NoError(err) // Create new OFAC checker testChecker, err := chain.NewOfacChecker() suite.NoError(err) // Verify that no address is blacklisted suite.False(testChecker.IsBlacklisted(suite.senderAddress.String())) suite.False(testChecker.IsBlacklisted("inj1")) }client/chain/chain_test.go (1)
Line range hint
71-73: Correct typos in variable names for consistencyThere are typos in the variable names
expectedSubaccounOnetIdandexpectedSubaccounThirtytId. They should be corrected toexpectedSubaccountOneIdandexpectedSubaccountThirtyIdfor clarity and consistency.Apply this diff to fix the variable names:
- expectedSubaccounOnetId := "0xaf79152ac5df276d9a8e1e2e22822f9713474902000000000000000000000001" - expectedSubaccounThirtytId := "0xaf79152ac5df276d9a8e1e2e22822f971347490200000000000000000000001e" + expectedSubaccountOneId := "0xaf79152ac5df276d9a8e1e2e22822f9713474902000000000000000000000001" + expectedSubaccountThirtyId := "0xaf79152ac5df276d9a8e1e2e22822f971347490200000000000000000000001e"Also, update the corresponding hash variable names and usages:
- expectedSubaccountOneIdHash := eth.HexToHash(expectedSubaccounOnetId) - expectedSubaccountThirtyIdHash := eth.HexToHash(expectedSubaccounThirtytId) + expectedSubaccountOneIdHash := eth.HexToHash(expectedSubaccountOneId) + expectedSubaccountThirtyIdHash := eth.HexToHash(expectedSubaccountThirtyId)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- client/chain/chain_test.go (3 hunks)
- client/chain/ofac.go (4 hunks)
- client/chain/ofac_test.go (1 hunks)
- injective_data/ofac.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
injective_data/ofac.json (2)
2-47: Ethereum addresses are in the correct format.All Ethereum addresses in the list are correctly formatted:
- They are 42 characters long (including the "0x" prefix).
- They use lowercase hexadecimal characters (0-9 and a-f).
- They all start with the "0x" prefix.
The consistent and correct formatting of addresses is crucial for accurate matching and reduces the risk of false negatives in OFAC checks.
1-48: Verify the completeness and accuracy of the OFAC list.The OFAC list is a critical component for compliance with U.S. sanctions. It's essential to ensure that this list is complete, up-to-date, and accurately reflects the current OFAC Specially Designated Nationals (SDN) list.
Please confirm the following:
- The source and accuracy of this list of Ethereum addresses.
- The process for keeping this list up-to-date with the official OFAC SDN list.
- Any legal or compliance review that has been conducted on the use of this list.
Consider adding comments or documentation explaining the process for maintaining and updating this list to ensure ongoing compliance.
client/chain/ofac_test.go (4)
3-15: LGTM: Import statements are well-organized and relevant.The import statements are comprehensive and appropriate for the test suite's requirements. They include necessary packages from the SDK, testing frameworks, and standard libraries.
17-24: LGTM: OfacTestSuite struct is well-defined.The
OfacTestSuitestruct is appropriately designed for OFAC-related testing. It includes all necessary fields for network configuration, client setup, and address management. TheoriginalOfacPathfield indicates proper handling of the OFAC list path during tests.
55-57: LGTM: TearDownTest method effectively restores the original state.The
TearDownTestmethod correctly restores the original OFAC list path after the tests are completed. This ensures that the testing environment doesn't interfere with other tests or the actual application state.
1-83: Overall: Well-structured and comprehensive OFAC test suite.This new test file introduces a robust suite for testing OFAC functionality. The code is clean, well-organized, and follows good testing practices. It effectively covers the main OFAC checking scenarios and includes proper setup and teardown methods.
Key strengths:
- Comprehensive setup and teardown processes.
- Effective testing of both positive and negative OFAC checking cases.
- Good error handling throughout the tests.
Suggestions for improvement:
- Consider adding more test cases to increase coverage, as mentioned in the previous comment.
- Use
deferfor file closing in theSetupTestmethod.Overall, this is a solid addition to the test suite that will help ensure the reliability of the OFAC checking functionality.
client/chain/chain_test.go (1)
1-1: Verify the impact of changing the package name to 'chain_test'Changing the package name from
chaintochain_testmeans your tests will run as an external package, only accessing exported identifiers from thechainpackage. Please ensure that all necessary identifiers are exported and that your tests function correctly after this change.
| [ | ||
| "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.
🛠️ Refactor suggestion
Consider adding metadata and versioning to the OFAC list.
While the current format is simple and easy to parse, it lacks important metadata such as:
- The date when the list was last updated
- A version number for the list
- A source or reference for the OFAC data
Consider modifying the JSON structure to include metadata. Here's an example of how you could structure this:
{
"version": "1.0.0",
"last_updated": "2023-05-15",
"source": "https://example.com/ofac_list",
"addresses": [
"0x179f48c78f57a3a78f0608cc9197b8972921d1d2",
// ... rest of the addresses ...
]
}This structure would provide more context and traceability for the OFAC list, which is crucial for compliance and auditing purposes.
| @@ -30,9 +27,9 @@ func accountForTests() (cosmtypes.AccAddress, keyring.Keyring, error) { | |||
| return senderAddress, cosmosKeyring, err | |||
| } | |||
|
|
|||
| func createClient(senderAddress cosmtypes.AccAddress, cosmosKeyring keyring.Keyring, network common.Network) (ChainClient, error) { | |||
| func createClient(senderAddress cosmtypes.AccAddress, cosmosKeyring keyring.Keyring, network common.Network) (chain.ChainClient, error) { | |||
| tmClient, _ := rpchttp.New(network.TmEndpoint, "/websocket") | |||
There was a problem hiding this comment.
Handle the error returned by rpchttp.New
The error returned by rpchttp.New is currently being ignored. It's important to handle this error to prevent potential runtime issues if the Tendermint RPC client fails to initialize.
Apply this diff to handle the error:
- tmClient, _ := rpchttp.New(network.TmEndpoint, "/websocket")
+ tmClient, err := rpchttp.New(network.TmEndpoint, "/websocket")
+ if err != nil {
+ return nil, err
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
client/chain/ofac.go (3)
13-13: LGTM, but consider URL reliability and security.The constant
DefaultOfacListURLis correctly declared and follows Go naming conventions. However, relying on an external URL for critical data like the OFAC list may introduce reliability and security concerns.Consider implementing the following improvements:
- Add a fallback mechanism in case the GitHub URL is unreachable.
- Implement URL validation and HTTPS certificate verification when downloading the list.
- Consider periodically caching the list locally to reduce dependency on external resources.
17-18: LGTM. Consider adding comments for clarity.The variables
OfacListPathandOfacListFilenameare correctly declared and follow Go naming conventions. The use of a relative path forOfacListPathaligns with the PR objective.Consider adding comments to explain the purpose of these variables and any constraints on their values. For example:
// OfacListPath is the relative path where the OFAC list file will be stored var OfacListPath = "injective_data" // OfacListFilename is the name of the OFAC list file var OfacListFilename = "ofac.json"
46-46: LGTM. Good practice to ensure directory exists.The changes in the
DownloadOfacListfunction are well-implemented:
- The use of
DefaultOfacListURLis consistent with earlier changes.- Creating the directory before file creation prevents potential errors.
Consider combining error checks to reduce nesting and improve readability:
if err := os.MkdirAll(OfacListPath, 0755); err != nil { // nolint:gocritic // 0755 is the correct permission return fmt.Errorf("failed to create directory: %w", err) } outFile, err := os.Create(GetOfacListPath()) if err != nil { return fmt.Errorf("failed to create file: %w", err) } defer outFile.Close()Also applies to: 56-59
client/chain/chain_test.go (1)
Line range hint
1-106: Summary of changes and recommendations.The changes in this file effectively support the PR objective of making the OFAC list file path relative. The test file has been successfully updated to work with the new package structure, and the removal of OFAC-specific tests is consistent with this change.
Key points:
- The package name and import statements have been correctly updated.
- Functions have been modified to use the new
chainpackage structure.- Test logic remains intact, ensuring continued verification of subaccount calculations.
One issue remains to be addressed:
- Error handling in the
createClientfunction forrpchttp.Newcall.Once the error handling issue is resolved, the changes in this file will be fully aligned with the PR objectives and best practices.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- client/chain/chain_test.go (3 hunks)
- client/chain/ofac.go (4 hunks)
- client/chain/ofac_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/chain/ofac_test.go
🧰 Additional context used
🔇 Additional comments (8)
client/chain/ofac.go (3)
28-28: LGTM. Consistent with other changes.The update to use
GetOfacListPath()is correct and consistent with the function renaming.
41-43: LGTM. Well-implemented path construction.The
GetOfacListPathfunction is correctly implemented:
- It follows Go naming conventions for exported functions.
- It uses
path.Joinfor cross-platform compatible path construction.- It encapsulates the path construction logic, improving maintainability.
Line range hint
1-95: Overall, the changes effectively address the PR objectives.The modifications in this file successfully make the OFAC list file path relative, addressing the issue of the file not being included in the compiled application. The changes consistently improve the structure, naming conventions, and exportability of OFAC-related code.
Key improvements:
- Consistent naming of exported constants and variables.
- Introduction of
GetOfacListPathfunction for centralized path management.- Ensuring the directory exists before file operations in
DownloadOfacList.These changes should allow applications using the SDK to function correctly without requiring a Go environment, as per the PR objectives.
client/chain/chain_test.go (5)
1-14: LGTM: Package and import changes look good.The package name change to
chain_testis appropriate for a test file. The import statements have been correctly updated to reflect the new structure, removing unused imports and adding necessary ones from thegithub.com/InjectiveLabs/sdk-go/clientpackage.
Line range hint
17-28: LGTM:accountForTestsfunction updated correctly.The
accountForTestsfunction has been properly updated to usechain.InitCosmosKeyring, aligning with the new package structure. The function signature and return values remain consistent, maintaining compatibility.
Line range hint
31-52: Address error handling and approve other changes.The
createClientfunction has been correctly updated to usechain.NewClientContextandchain.NewChainClient, aligning with the new package structure. The return type change tochain.ChainClientis also appropriate.However, there's still an unresolved issue:
The error returned by
rpchttp.Newis still being ignored. Please handle this error to prevent potential runtime issues if the Tendermint RPC client fails to initialize.Apply this diff to handle the error:
- tmClient, _ := rpchttp.New(network.TmEndpoint, "/websocket") + tmClient, err := rpchttp.New(network.TmEndpoint, "/websocket") + if err != nil { + return nil, err + }
Line range hint
54-77: LGTM:TestDefaultSubaccountupdated correctly.The
TestDefaultSubaccountfunction has been properly updated to use the newcreateClientfunction. The test logic remains intact, ensuring that the default subaccount calculation is still being verified correctly.
Line range hint
79-106: LGTM:TestGetSubaccountWithIndexupdated correctly.The
TestGetSubaccountWithIndexfunction has been properly updated to use the newcreateClientfunction. The test logic remains intact, correctly verifying the calculation of subaccounts with indices 1 and 30.
ofac.jsonfile was not included in the compiled app, and if the app was executed in a different environment than the one used to compile it, the path to the json file was still referencing the path at compile timeSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation