refactor: configure solana chain during e2e setup #2990
Conversation
gartnera
added
no-changelog
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2990 +/- ##
===========================================
- Coverage 66.08% 66.05% -0.04%
===========================================
Files 397 397
Lines 22334 22313 -21
===========================================
- Hits 14760 14739 -21
Misses 6806 6806
Partials 768 768
|
gartnera
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
📝 WalkthroughWalkthroughThe pull request introduces significant updates to the handling of Solana contracts within the codebase. Key changes include renaming methods related to Solana contract setup, enhancing transaction status checks, and removing references to Solana chain parameters from various files. The modifications streamline the initialization logic for local testing environments and adjust the focus away from Solana, aligning the codebase with current requirements. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (6)
e2e/e2etests/test_solana_withdraw.go (2)
23-23: Enhance error message for insufficient balance check.While the balance check is crucial, consider improving the error message to provide more context:
require.Equal(r, 1, balanceBefore.Cmp(withdrawAmount), "Insufficient balance for withdrawal. Available: %s, Requested: %s", balanceBefore.String(), withdrawAmount.String())This modification will offer more detailed information in case of a test failure.
31-31: Refine error message for withdrawal amount comparison.To enhance clarity, consider modifying the error message as follows:
"Withdrawal amount (%s) must be less than the approved amount (1 SOL = %s lamports)", withdrawAmount.String(), approvedAmount.String()This change provides more context and uses the actual values in the error message, aiding in debugging.
e2e/runner/solana.go (1)
165-165: Approval: Consistent enhancement of transaction context.The addition of the "withdraw" parameter to
utils.RequireTxSuccessfulis a logical and consistent improvement, mirroring the earlier change. This modification further enhances the clarity and debuggability of the code.To ensure consistency across the codebase, consider the following suggestion:
// Define constants for transaction types const ( TxTypeApprove = "approve" TxTypeWithdraw = "withdraw" // Add other transaction types as needed ) // Use constants in function calls utils.RequireTxSuccessful(r, receipt, TxTypeApprove) utils.RequireTxSuccessful(r, receipt, TxTypeWithdraw)This refactoring would centralize transaction type definitions, reducing the risk of typos and improving maintainability.
zetaclient/context/app_test.go (1)
116-116: Approve the removal of Solana chain ID with a minor suggestion.The removal of the Solana chain ID from the
expectedIDsslice is consistent with the PR objectives and the overall changes to the codebase. This modification appropriately adjusts the test case to focus on the supported chains: Ethereum, Bitcoin, and the custom fancy L2 chain.To enhance clarity and maintainability, consider using constants or variables for the chain IDs instead of hardcoding them. This approach would make the test more resilient to future changes in chain configurations.
Consider refactoring the line as follows:
expectedIDs := []int64{chains.Ethereum.ChainId, chains.BitcoinMainnet.ChainId, fancyL2.ChainId}This change would make the test more robust and easier to maintain as it directly references the chain ID constants.
cmd/zetae2e/local/local.go (1)
229-229: Approve the change toSetupSolanaand suggest a minor improvement.The modification from
SetSolanaContractstoSetupSolanaaligns with the PR objectives of configuring the Solana chain during setup. This change is likely to address the reported issues with RPC calls to the Solana network during standard e2e runs.To enhance clarity and maintainability, consider adding a brief comment explaining the purpose of this setup step:
if testSolana { + // Configure Solana chain and deploy necessary contracts deployerRunner.SetupSolana(conf.AdditionalAccounts.UserSolana.SolanaPrivateKey.String()) }This comment will help future developers understand the significance of this step in the e2e test setup process.
e2e/runner/setup_solana.go (1)
4-5: Import Statements: Organize Imports According to Go ConventionsThe added import
"time"should be grouped appropriately. According to Go conventions, standard library imports should be separated from third-party and project-specific imports.Consider organizing the imports as follows:
package runner -import ( - "time" - - ethcommon "github.com/ethereum/go-ethereum/common" - "github.com/gagliardetto/solana-go" - "github.com/gagliardetto/solana-go/rpc" - "github.com/near/borsh-go" - "github.com/pkg/errors" - "github.com/stretchr/testify/require" - - "github.com/zeta-chain/node/e2e/utils" - "github.com/zeta-chain/node/pkg/chains" - "github.com/zeta-chain/node/pkg/constant" - solanacontracts "github.com/zeta-chain/node/pkg/contracts/solana" - observertypes "github.com/zeta-chain/node/x/observer/types" -) +import ( + "time" + + ethcommon "github.com/ethereum/go-ethereum/common" + "github.com/gagliardetto/solana-go" + "github.com/gagliardetto/solana-go/rpc" + "github.com/near/borsh-go" + "github.com/pkg/errors" + "github.com/stretchr/testify/require" + + "github.com/zeta-chain/node/e2e/utils" + "github.com/zeta-chain/node/pkg/chains" + "github.com/zeta-chain/node/pkg/constant" + solanacontracts "github.com/zeta-chain/node/pkg/contracts/solana" + observertypes "github.com/zeta-chain/node/x/observer/types" +)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- cmd/zetae2e/local/local.go (1 hunks)
- e2e/e2etests/test_solana_withdraw.go (1 hunks)
- e2e/runner/setup_solana.go (3 hunks)
- e2e/runner/solana.go (2 hunks)
- x/observer/genesis.go (0 hunks)
- x/observer/genesis_test.go (0 hunks)
- x/observer/types/chain_params.go (0 hunks)
- zetaclient/context/app_test.go (1 hunks)
💤 Files with no reviewable changes (3)
- x/observer/genesis.go
- x/observer/genesis_test.go
- x/observer/types/chain_params.go
🧰 Additional context used
📓 Path-based instructions (5)
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/test_solana_withdraw.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/setup_solana.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.zetaclient/context/app_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (5)
e2e/e2etests/test_solana_withdraw.go (1)
16-17: Commendable introduction of a descriptive variable.The addition of
withdrawAmountenhances code clarity and separates concerns effectively.e2e/runner/solana.go (2)
156-156: Approval: Enhanced transaction context provided.The addition of the "approve" parameter to
utils.RequireTxSuccessfulis a commendable improvement. It enhances the clarity and debuggability of the code by providing specific context for the transaction being verified. This modification aligns with best practices for error handling and logging in production-grade software.
Line range hint
1-170: Summary: Improvements in transaction context and suggestion for further enhancement.The changes made to the
WithdrawSOLZRC20method ine2e/runner/solana.goare beneficial, enhancing code clarity and debuggability. The consistent addition of transaction type parameters toutils.RequireTxSuccessfulcalls is commendable.To further elevate the code quality:
- Consider implementing the suggested refactoring using constants for transaction types.
- Ensure this pattern is applied consistently across the entire codebase where similar transaction verifications occur.
These modifications contribute to a more robust and maintainable e2e testing framework for Solana interactions.
zetaclient/context/app_test.go (1)
Line range hint
1-265: Overall assessment: Changes are consistent and maintain test integrity.The modifications to this test file, particularly the removal of Solana-related code, are consistent with the PR objectives. The changes maintain the integrity of the test cases for the supported chains (Ethereum, Bitcoin, and the custom fancy L2 chain) while removing unnecessary Solana-specific tests.
The overall structure and purpose of the test file remain intact, ensuring that the
AppContextfunctionality is still thoroughly tested for the remaining supported chains. No further modifications appear necessary for this file.e2e/runner/setup_solana.go (1)
29-30: Function Renaming: Ensure Consistency Across the CodebaseThe function
SetSolanaContractshas been renamed toSetupSolana. Please ensure that all references to the old function name throughout the codebase have been updated to prevent potential runtime errors.Run the following script to find any outdated references:
gartnera
force-pushed
the
solana-dynamic-chain-params
branch
from
5faa2f9 to
5f48e79
Compare
gartnera
changed the title
gartnera
changed the title
Description
Configure solana during e2e setup rather than at genesis to avoid these zetaclient error messages during standard e2e run:
This matches how TON is setup but we also reset the chain nonces like https://github.com/zeta-chain/gov-ops/pull/424
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Chores