test: Solana e2e deposit and call; deposit and revert #2726
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe recent changes enhance the testing framework for Solana interactions, introducing new end-to-end tests for deposit, call, and refund scenarios. The updates improve the robustness of the testing coverage by refining the deposit processing logic, adding balance checks, and enhancing error handling mechanisms. Overall, these modifications aim to ensure the reliability and accuracy of cross-chain operations within the project. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SolanaContract
participant E2ERunner
User->>E2ERunner: Initiate deposit
E2ERunner->>SolanaContract: Create deposit instruction
E2ERunner->>SolanaContract: Broadcast transaction
SolanaContract->>E2ERunner: Confirm transaction
E2ERunner->>User: Return transaction status
Assessment against linked issues
Possibly related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2726 +/- ##
========================================
Coverage 66.90% 66.90%
========================================
Files 364 364
Lines 20458 20458
========================================
Hits 13688 13688
Misses 6143 6143
Partials 627 627 |
ws4charlie
changed the title
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 13340122 | Triggered | Generic High Entropy Secret | 96351b4 | zetaclient/chains/solana/signer/signer_test.go | View secret |
| 13340122 | Triggered | Generic High Entropy Secret | 96351b4 | zetaclient/chains/solana/signer/signer_test.go | View secret |
| 13392123 | Triggered | Generic High Entropy Secret | 96351b4 | zetaclient/chains/solana/signer/signer_test.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
![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, codebase verification and nitpick comments (27)
e2e/utils/contracts.go (1)
17-32: Ensure comprehensive error handling and logging.The function
MustHaveCalledExampleContractcorrectly checks the contract call result. However, consider adding logging for successful operations to improve traceability. Additionally, ensure that the error message is informative enough for debugging.+ r.Logger.Info("Successfully called ExampleContract with amount: %s", amount.String())e2e/e2etests/test_solana_withdraw.go (2)
16-19: Clarify logging messages for better context.The logging message indicates the balance before withdrawal. Ensure that the message provides clear context, such as specifying the currency or token type.
- r.Logger.Info("runner balance of SOL before withdraw: %d", balanceBefore) + r.Logger.Info("Runner's ERC20 SOL balance before withdrawal: %d", balanceBefore)
39-42: Enhance logging message for post-withdrawal balance.The logging message should clearly indicate that it refers to the balance after the withdrawal operation.
- r.Logger.Info("runner balance of SOL after withdraw: %d", balanceAfter) + r.Logger.Info("Runner's ERC20 SOL balance after withdrawal: %d", balanceAfter)e2e/e2etests/test_solana_deposit.go (2)
18-21: Improve logging clarity for pre-deposit balance.The logging message should specify the currency or token type for clarity.
- r.Logger.Info("runner balance of SOL before deposit: %d", balanceBefore) + r.Logger.Info("Runner's ERC20 SOL balance before deposit: %d", balanceBefore)
45-48: Clarify logging message for post-deposit balance.The logging message should clearly indicate that it refers to the balance after the deposit operation.
- r.Logger.Info("runner balance of SOL after deposit: %d", balanceAfter) + r.Logger.Info("Runner's ERC20 SOL balance after deposit: %d", balanceAfter)e2e/e2etests/test_solana_deposit_call.go (7)
15-16: Clarify function purpose in the comment.The function comment should clearly describe the purpose and scope of the test, including any specific conditions or scenarios it covers. Consider expanding the comment to include this information.
-// TestSolanaDepositAndCall tests deposit of lamports calling a example contract +// TestSolanaDepositAndCall verifies the deposit of lamports and the execution of a call to an example contract on Solana.
17-17: Validate argument length with a descriptive error message.The error message could be more descriptive to aid debugging if the argument length is incorrect.
-require.Len(r, args, 1) +require.Len(r, args, 1, "TestSolanaDepositAndCall requires exactly one argument: the deposit amount.")
24-24: Log deployment errors for better traceability.Enhance error handling by logging deployment errors to help with debugging.
require.NoError(r, err, "Failed to deploy example contract.")
31-31: Handle private key parsing errors explicitly.Consider logging the error when parsing the private key fails for better traceability.
require.NoError(r, err, "Failed to parse Solana private key.")
36-36: Clarify data usage in deposit instruction.Provide a brief comment explaining the purpose of the
datavariable to improve code readability.// Example data payload for the deposit instruction data := []byte("hello lamports")
43-43: Enhance log message clarity.The log message could be more descriptive to indicate the context of the logs.
r.Logger.Info("Deposit transaction logs: %v", out.Meta.LogMessages)
51-51: Ensure contract call verification is robust.Consider adding a descriptive error message to the
MustHaveCalledExampleContractfunction to clarify the failure context.utils.MustHaveCalledExampleContract(r, contract, depositAmount, "Example contract call verification failed.")e2e/e2etests/test_solana_deposit_refund.go (7)
15-16: Clarify function purpose in the comment.The function comment should clearly describe the purpose and scope of the test, including any specific conditions or scenarios it covers. Consider expanding the comment to include this information.
-// TestSolanaDepositAndCallRefund tests deposit of lamports calling a example contract +// TestSolanaDepositAndCallRefund verifies the deposit of lamports and the handling of a revert scenario with a reverter contract on Solana.
17-17: Validate argument length with a descriptive error message.The error message could be more descriptive to aid debugging if the argument length is incorrect.
-require.Len(r, args, 1) +require.Len(r, args, 1, "TestSolanaDepositAndCallRefund requires exactly one argument: the deposit amount.")
25-25: Log deployment errors for better traceability.Enhance error handling by logging deployment errors to help with debugging.
require.NoError(r, err, "Failed to deploy reverter contract.")
32-32: Handle private key parsing errors explicitly.Consider logging the error when parsing the private key fails for better traceability.
require.NoError(r, err, "Failed to parse Solana private key.")
37-37: Clarify data usage in deposit instruction.Provide a brief comment explaining the purpose of the
datavariable to improve code readability.// Example data payload for the deposit instruction data := []byte("hello reverter")
44-44: Enhance log message clarity.The log message could be more descriptive to indicate the context of the logs.
r.Logger.Info("Deposit transaction logs: %v", out.Meta.LogMessages)
53-53: Ensure revert status message verification is robust.Consider adding a descriptive error message to the
require.Containsfunction to clarify the failure context.require.Contains(r, cctx.CctxStatus.StatusMessage, utils.ErrHashRevert, "Revert error hash not found in status message.")e2e/e2etests/test_eth_deposit_call.go (6)
Line range hint
13-13:
Clarify function purpose in the comment.The function comment should clearly describe the purpose and scope of the test, including any specific conditions or scenarios it covers. Consider expanding the comment to include this information.
-// TestEtherDepositAndCall tests deposit of ethers calling a example contract +// TestEtherDepositAndCall verifies the deposit of ethers and the execution of a call to an example contract on Ethereum.
Line range hint
15-15:
Validate argument length with a descriptive error message.The error message could be more descriptive to aid debugging if the argument length is incorrect.
-require.Len(r, args, 1) +require.Len(r, args, 1, "TestEtherDepositAndCall requires exactly one argument: the deposit amount.")
Line range hint
17-18:
Ensure safe parsing of deposit amount.Consider adding error handling for the
SetStringfunction to ensure robustness in case of invalid input.value, ok := big.NewInt(0).SetString(args[0], 10) require.True(r, ok, "Invalid amount specified for TestEtherDepositAndCall.")
Line range hint
26-26:
Log deployment errors for better traceability.Enhance error handling by logging deployment errors to help with debugging.
require.NoError(r, err, "Failed to deploy example contract.")
59-59: Ensure contract call verification is robust.Consider adding a descriptive error message to the
MustHaveCalledExampleContractfunction to clarify the failure context.utils.MustHaveCalledExampleContract(r, exampleContract, value, "Example contract call verification failed.")
93-93: Ensure revert status message verification is robust.Consider adding a descriptive error message to the
require.Containsfunction to clarify the failure context.require.Contains(r, cctx.CctxStatus.StatusMessage, utils.ErrHashRevert, "Revert error hash not found in status message.")changelog.md (2)
21-23: Clarify the test description.The entry "add e2e tests for deposit and call, deposit and revert" is concise but could benefit from additional context or detail to clarify what these tests are verifying or why they are significant.
- * [2726](https://github.com/zeta-chain/node/pull/2726) - add e2e tests for deposit and call, deposit and revert + * [2726](https://github.com/zeta-chain/node/pull/2726) - add end-to-end tests for Solana's deposit and call, and deposit and revert functionalities, ensuring robust cross-chain operations.
24-24: Ensure clarity and consistency in fix descriptions.The entry "add validation for authorization list in when validating genesis state for authorization module" could be clearer. Consider revising for grammatical correctness and clarity.
- * [2654](https://github.com/zeta-chain/node/pull/2654) - add validation for authorization list in when validating genesis state for authorization module + * [2654](https://github.com/zeta-chain/node/pull/2654) - add validation for the authorization list during genesis state validation for the authorization module.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- changelog.md (1 hunks)
- cmd/zetae2e/local/local.go (1 hunks)
- e2e/e2etests/e2etests.go (2 hunks)
- e2e/e2etests/test_eth_deposit_call.go (3 hunks)
- e2e/e2etests/test_solana_deposit.go (4 hunks)
- e2e/e2etests/test_solana_deposit_call.go (1 hunks)
- e2e/e2etests/test_solana_deposit_refund.go (1 hunks)
- e2e/e2etests/test_solana_withdraw.go (2 hunks)
- e2e/runner/solana.go (3 hunks)
- e2e/utils/contracts.go (1 hunks)
- x/fungible/keeper/evm_test.go (3 hunks)
Additional context used
Path-based instructions (10)
e2e/utils/contracts.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_solana_withdraw.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_solana_deposit.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_solana_deposit_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_solana_deposit_refund.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_eth_deposit_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/solana.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetae2e/local/local.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/e2etests.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/fungible/keeper/evm_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (7)
e2e/e2etests/test_solana_deposit.go (1)
32-32: Verify the impact of the modified function signature.The
CreateDepositInstructionfunction now includes an additionalnilparameter. Ensure that this change is reflected in all relevant parts of the codebase and does not introduce unintended side effects.e2e/runner/solana.go (2)
100-109: LGTM! Timeout mechanism is a robust addition.The inclusion of a timeout in
BroadcastTxSyncenhances the robustness of transaction handling by preventing indefinite waits.
Line range hint
34-55:
LGTM! Verify the usage of the updated method.The changes to include a
dataparameter inCreateDepositInstructionenhance the flexibility of memo construction.Ensure that all calls to this method in the codebase are updated to pass the new
dataparameter.Verification successful
Verification Successful: All calls to
CreateDepositInstructionare updated correctly.The implementation of the
CreateDepositInstructionmethod has been successfully updated across the codebase to include thedataparameter. The following files reflect the correct usage:
e2e/e2etests/test_solana_deposit_call.goe2e/e2etests/test_solana_deposit_refund.goe2e/e2etests/test_solana_deposit.goAll instances pass the
dataparameter, ensuring the method's new signature is consistently applied.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `CreateDepositInstruction` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'CreateDepositInstruction'Length of output: 1990
cmd/zetae2e/local/local.go (1)
335-336: LGTM! New test cases are well integrated.The addition of
TestSolanaDepositAndCallNameandTestSolanaDepositAndCallRefundNameexpands the Solana test coverage effectively.e2e/e2etests/e2etests.go (2)
59-60: LGTM! New constants for Solana tests are well defined.The addition of
TestSolanaDepositAndCallNameandTestSolanaDepositAndCallRefundNameenhances the test suite.
346-371: Verify the updated default values for Solana tests.The default values for
TestSolanaDepositandTestSolanaWithdrawhave been updated. Ensure these values align with the expected test scenarios.Verification successful
Verification of Default Values for Solana Tests
The default values for
TestSolanaDepositandTestSolanaWithdrawhave been updated to "1200000" and "1000000" lamports, respectively. These values are correctly used within the test functions, aligning with the expected scenarios. No issues were found in their implementation.
TestSolanaDeposituses the default value "1200000" lamports.TestSolanaWithdrawuses the default value "1000000" lamports.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the updated default values for Solana tests align with the expected scenarios. # Test: Search for the usage of `TestSolanaDeposit` and `TestSolanaWithdraw`. Expect: Correct handling of updated values. rg --type go -A 5 $'TestSolanaDeposit|TestSolanaWithdraw'Length of output: 5127
x/fungible/keeper/evm_test.go (1)
579-579: Ensure correct usage of dynamic error messages.The use of
utils.ErrHashRevertfor dynamic error messages enhances maintainability. Ensure thatutils.ErrHashRevertis correctly defined and used consistently across the codebase.Verification successful
Dynamic error message usage is correct and consistent.
The variable
utils.ErrHashRevertis defined and used consistently across the codebase, enhancing maintainability by avoiding hardcoded error messages.
- Definition:
e2e/utils/contracts.go- Usage:
x/fungible/keeper/evm_test.go,e2e/e2etests/test_solana_deposit_refund.go,e2e/e2etests/test_eth_deposit_call.goThis confirms the correct implementation of dynamic error handling in the test.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of `utils.ErrHashRevert`. # Test: Search for the definition and usage of `ErrHashRevert`. Expect: Consistent and correct usage. rg --type go 'ErrHashRevert'Length of output: 526
Description
Done:
zetae2eCLICloses: #2659
Closes: #2507
Closes: #2508
Live tests result in localnet:
solana_depositsolana_withdrawsolana_deposit_and_callsolana_deposit_and_call_refundHow Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Chores