Fix/sync dev after v1 53 0 #252
Conversation
WalkthroughThe pull request introduces several modifications primarily focused on error handling in the Changes
Possibly related PRs
Suggested reviewers
Poem
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
|
…ps to be able to read/download the file
…ved the test to use a temp test directory
PavelInjective
force-pushed
the
fix/sync_dev_after_v1_53_0
branch
from
4ae8d48 to
722a2d3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (13)
client/chain/ofac.go (3)
13-13: LGTM! Consider using a versioned URL.The constant
DefaultOfacListURLis well-named and follows Go conventions. However, consider using a versioned URL or a more stable endpoint to prevent potential issues if the repository structure changes.
17-18: LGTM! Consider environment-specific configurations.The variables
OfacListPathandOfacListFilenameare now exported, which is good for reusability. However, consider the following improvements:
- Use environment variables or configuration files to set
OfacListPathfor different environments.- Add comments explaining the purpose of these variables and their expected values.
Line range hint
41-59: LGTM! Good improvements in error handling and directory creation.The new
GetOfacListPathfunction and the updates toDownloadOfacListare well-implemented. The addition of directory creation is a good improvement. A few suggestions:
- Consider adding a comment explaining why
0755permissions are used and why the linter exception is necessary.- The error handling for
os.MkdirAllcould be combined with theos.Createerror handling to reduce nesting.Here's a suggestion for combining error handling:
if err := os.MkdirAll(OfacListPath, 0755); err != nil { 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()injective_data/ofac.json (1)
1-48: Consider the implications of maintaining an OFAC-related address list.The file name "ofac.json" suggests this list is related to the Office of Foreign Assets Control (OFAC). This list likely serves a compliance purpose, possibly to check transactions against sanctioned addresses. Consider the following implications:
- Regulatory compliance: Ensure your system uses this list effectively to comply with OFAC regulations.
- Privacy: Verify that storing and using this list complies with relevant data protection regulations.
- Maintenance: Implement a process for regular updates to reflect the latest OFAC sanctions.
- Performance: Optimize the application for quick lookups against this list, especially if used in transaction processing.
Consider implementing a more robust solution for managing and using this list, such as:
- Using a database instead of a JSON file for better performance and easier updates.
- Implementing a background job for regular updates from an authoritative source.
- Adding logging and alerting mechanisms for when transactions involve listed addresses.
client/chain/ofac_test.go (2)
27-54: LGTM with a minor suggestion: Consider improving error handling in file operations.The
SetupTestmethod thoroughly initializes the test environment, including network setup and OFAC list preparation. However, consider usingdeferfor file closing to ensure it happens even if an error occurs.Consider modifying the file handling part as follows:
file, err := os.Create(chain.GetOfacListPath()) suite.NoError(err) +defer func() { + cerr := file.Close() + if err == nil { + err = cerr + } +}() _, err = io.WriteString(file, string(jsonData)) suite.NoError(err) -err = file.Close() -suite.NoError(err)This ensures that the file is always closed and any error during closing is properly handled.
56-80: LGTM with a suggestion: Consider expanding test coverage inTestOfacList.The
TearDownTestandTestOfacListmethods are well-implemented. The OFAC checking functionality and client creation are properly tested. However, consider adding more test cases toTestOfacListfor a more comprehensive coverage.Consider adding the following test cases to
TestOfacList:
- Test with multiple addresses in the OFAC list.
- Test the behavior when the OFAC list is empty.
- Test with invalid addresses.
- Test the performance with a large OFAC list.
Example:
func (suite *OfacTestSuite) TestOfacList() { // ... existing code ... // Test with multiple addresses multipleAddresses := []string{suite.senderAddress.String(), "inj1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq"} testChecker, err = chain.NewOfacChecker() suite.NoError(err) for _, addr := range multipleAddresses { suite.True(testChecker.IsBlacklisted(addr)) } // Test with empty OFAC list // ... implement this test case // Test with invalid address suite.False(testChecker.IsBlacklisted("invalid_address")) // Test performance with large OFAC list // ... implement this test case }client/chain/chain_test.go (7)
Line range hint
17-24: Use a temporary directory for keyring storage in testsUsing a hardcoded path like
os.Getenv("HOME")+"/.injectived"for the keyring storage can lead to conflicts or issues when running tests in different environments or concurrently. It's better to use a temporary directory to ensure isolation and prevent side effects.Apply this change:
-func accountForTests() (cosmtypes.AccAddress, keyring.Keyring, error) { +func accountForTests(t *testing.T) (cosmtypes.AccAddress, keyring.Keyring, error) { + tempDir := t.TempDir() senderAddress, cosmosKeyring, err := chain.InitCosmosKeyring( - os.Getenv("HOME")+"/.injectived", + tempDir, "injectived", "file", "inj-user", "12345678", "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", // keyring will be used if pk not provided false, )Note: You will need to pass
t *testing.TtoaccountForTestsand update any calls to this function accordingly.
Line range hint
55-57: Uset.Fatalfinstead oft.Errorffor critical setup errorsWhen an error occurs during test setup, such as failing to create the address, it's appropriate to use
t.Fatalfto immediately terminate the test. Usingt.Errorflogs the error but allows the test to continue, which may lead to misleading results or panics.Apply this change:
if err != nil { - t.Errorf("Error creating the address %v", err) + t.Fatalf("Error creating the address: %v", err) }
Line range hint
61-63: Uset.Fatalfinstead oft.Errorfwhen failing to create the clientFailing to create the client is a critical error that should halt the test immediately to prevent subsequent failures or misleading results.
Apply this change:
if err != nil { - t.Errorf("Error creating the client %v", err) + t.Fatalf("Error creating the client: %v", err) }
Line range hint
77-79: Uset.Fatalfinstead oft.Errorffor critical setup errorsConsistently use
t.Fatalfwhen setup errors occur to ensure the test stops immediately, maintaining test reliability.Apply this change:
if err != nil { - t.Errorf("Error creating the address %v", err) + t.Fatalf("Error creating the address: %v", err) }
Line range hint
83-85: Uset.Fatalfinstead oft.Errorfwhen failing to create the clientAs with the previous test, use
t.Fatalfto handle critical errors appropriately.Apply this change:
if err != nil { - t.Errorf("Error creating the client %v", err) + t.Fatalf("Error creating the client: %v", err) }
Line range hint
91-96: Correct typos in variable namesThere are typos in the variable names that could lead to confusion or errors:
expectedSubaccounOnetIdshould beexpectedSubaccountOneIdexpectedSubaccounThirtytIdshould beexpectedSubaccountThirtyIdApply this change:
-expectedSubaccounOnetId := "0xaf79152ac5df276d9a8e1e2e22822f9713474902000000000000000000000001" -expectedSubaccountOneIdHash := eth.HexToHash(expectedSubaccounOnetId) +expectedSubaccountOneId := "0xaf79152ac5df276d9a8e1e2e22822f9713474902000000000000000000000001" +expectedSubaccountOneIdHash := eth.HexToHash(expectedSubaccountOneId) -expectedSubaccounThirtytId := "0xaf79152ac5df276d9a8e1e2e22822f971347490200000000000000000000001e" -expectedSubaccountThirtyIdHash := eth.HexToHash(expectedSubaccounThirtytId) +expectedSubaccountThirtyId := "0xaf79152ac5df276d9a8e1e2e22822f971347490200000000000000000000001e" +expectedSubaccountThirtyIdHash := eth.HexToHash(expectedSubaccountThirtyId)
Line range hint
17-24: Consider parameterizing test account details and network configurationThe tests currently use hardcoded account credentials and network configurations, which may not be suitable for all testing environments and could pose security risks if sensitive information is exposed.
Consider externalizing these details using environment variables or a configuration file. This approach enhances flexibility and security by allowing different test setups without modifying the code.
Also applies to: 55-63, 77-85
🛑 Comments failed to post (1)
injective_data/ofac.json (1)
1-48:
⚠️ Potential issueRemove duplicate Ethereum addresses.
There are several duplicate entries in the list:
- "0x19aa5fe80d33a56d56c78e82ea5e50e5d80b4dff" (lines 4 and 5)
- "0x2f389ce8bd8ff92de3402ffce4691d17fc4f6535" (lines 7 and 8)
- "0x4f47bc496083c727c5fbe3ce9cdf2b0f6496270c" (lines 14, 15, and 16)
- "0xb6f5ec1a0a9cd1526536d3f0426c429529471f40" (lines 33, 34, and 35)
- "0xd882cfc20f52f2599d84b8e8d58c7fb62cfe344b" (lines 39 and 40)
Consider removing these duplicates to improve efficiency and prevent potential issues in processing this list.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
client/chain/chain_test.go (2)
Line range hint
31-52: LGTM: Function signature and implementation updates are correct.The changes to
createClientfunction, including the return type update tochain.ChainClientand the use ofchain.NewClientContextandchain.NewChainClient, are appropriate given the package change. These modifications improve type safety and clarity.Consider adding error handling for the
tmClientcreation:-tmClient, _ := rpchttp.New(network.TmEndpoint, "/websocket") +tmClient, err := rpchttp.New(network.TmEndpoint, "/websocket") +if err != nil { + return nil, fmt.Errorf("failed to create tmClient: %w", err) +}This would ensure that any issues with creating the
tmClientare properly caught and reported.
Line range hint
78-106: LGTM: Test function updated correctly with a minor suggestion.The
TestGetSubaccountWithIndexfunction has been properly updated to use the newcreateClientfunction while maintaining the original test logic. This ensures that the existing test coverage is preserved while adapting to the package changes.For consistency, consider updating the variable names to use camelCase:
-expectedSubaccounOnetId := "0xaf79152ac5df276d9a8e1e2e22822f9713474902000000000000000000000001" -expectedSubaccountOneIdHash := eth.HexToHash(expectedSubaccounOnetId) +expectedSubaccountOneId := "0xaf79152ac5df276d9a8e1e2e22822f9713474902000000000000000000000001" +expectedSubaccountOneIdHash := eth.HexToHash(expectedSubaccountOneId) -expectedSubaccounThirtytId := "0xaf79152ac5df276d9a8e1e2e22822f971347490200000000000000000000001e" -expectedSubaccountThirtyIdHash := eth.HexToHash(expectedSubaccounThirtytId) +expectedSubaccountThirtyId := "0xaf79152ac5df276d9a8e1e2e22822f971347490200000000000000000000001e" +expectedSubaccountThirtyIdHash := eth.HexToHash(expectedSubaccountThirtyId)This would improve readability and maintain consistent naming conventions throughout the code.
📜 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)
🚧 Files skipped from review as they are similar to previous changes (3)
- client/chain/ofac.go
- client/chain/ofac_test.go
- injective_data/ofac.json
🧰 Additional context used
🔇 Additional comments (4)
client/chain/chain_test.go (4)
1-14: LGTM: Package and import changes are appropriate.The transition to
chain_testpackage and the addition of new imports align with the file's new purpose as a dedicated test file for thechainpackage. This change improves the separation of concerns and test organization.
Line range hint
16-28: LGTM: Improved code reuse inaccountForTests.The function now utilizes
chain.InitCosmosKeyring, which reduces code duplication and ensures consistency between the main implementation and tests. This is a good practice for maintaining code quality and reducing potential discrepancies.
Line range hint
54-76: LGTM: Test function updated correctly.The
TestDefaultSubaccountfunction has been properly updated to use the newcreateClientfunction while maintaining the original test logic. This ensures that the existing test coverage is preserved while adapting to the package changes.
Line range hint
1-106: Verify the removal of OFAC-related tests.The OFAC-related tests (
OfacTestSuiteand its methods) have been removed from this file. While this aligns with the file's transition to a dedicated test file for thechainpackage, it's important to ensure that these tests haven't been accidentally removed without being relocated elsewhere.Please confirm that the OFAC-related tests have been intentionally removed or relocated to an appropriate location. If they've been moved, could you provide information about their new location?
✅ Verification successful
OFAC-related tests have been successfully relocated.
The OFAC-related tests have been moved to
client/chain/ofac_test.go. The removal fromclient/chain/chain_test.gois intentional and correctly executed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for OFAC-related test files or test suites in the project echo "Searching for OFAC-related test files:" fd -e go | xargs rg -i "ofac.*test" echo "Searching for OfacTestSuite in other files:" rg -i "type\s+OfacTestSuite\s+struct"Length of output: 704
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests