fix: improve solana tx broadcasting in e2e tests #3378
Conversation
skosito
added
no-changelog
📝 WalkthroughWalkthroughThe pull request introduces comprehensive modifications to Solana-related end-to-end (e2e) testing infrastructure, focusing on enhancing transaction handling, commitment levels, and compute budget management. The changes primarily involve updating method calls across multiple files to use Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
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: 7
🧹 Nitpick comments (6)
e2e/runner/solana.go (1)
238-241: Optimize compute budget instructions placement.For clearer readability and potential reuse, consider grouping the compute budget instructions into a separate function or placing them at the beginning of the transaction construction.
e2e/e2etests/test_solana_whitelist_spl.go (1)
5-5: Organize imports according to Go conventions.Consider grouping standard library imports separately from third-party imports for improved readability.
Apply this diff to organize imports:
import ( "github.com/gagliardetto/solana-go" + "github.com/gagliardetto/solana-go/rpc" "github.com/stretchr/testify/require" "github.com/zeta-chain/node/e2e/runner"e2e/e2etests/test_spl_deposit.go (1)
48-51: Handle possible race conditions in balance assertions.Using
rpc.CommitmentConfirmedmight cause the balances to reflect unfinalized transactions. Ensure that the balance assertions account for possible delays in state updates.e2e/e2etests/test_spl_withdraw.go (1)
43-43: Consider adding retry mechanism for balance checksWhile switching to
CommitmentConfirmedhelps reduce transaction drops, it might introduce race conditions in balance verification. Consider implementing a retry mechanism with timeout for balance checks to ensure test reliability.+func getTokenBalanceWithRetry(client *rpc.Client, ctx context.Context, account solana.PublicKey, maxRetries int, interval time.Duration) (*rpc.GetTokenAccountBalanceResult, error) { + for i := 0; i < maxRetries; i++ { + balance, err := client.GetTokenAccountBalance(ctx, account, rpc.CommitmentConfirmed) + if err == nil { + return balance, nil + } + time.Sleep(interval) + } + return nil, fmt.Errorf("failed to get token balance after %d retries", maxRetries) +}Also applies to: 60-60
e2e/e2etests/test_spl_deposit_and_call.go (1)
32-32: Consolidate balance verification logicMultiple similar balance checks could be consolidated into a helper function to improve maintainability and reduce code duplication.
+func verifyTokenBalance(r *runner.E2ERunner, account solana.PublicKey, expectedDelta int64, isDecrease bool) error { + balanceBefore, err := r.SolanaClient.GetTokenAccountBalance(r.Ctx, account, rpc.CommitmentConfirmed) + if err != nil { + return fmt.Errorf("failed to get balance before: %w", err) + } + + // ... perform transaction ... + + balanceAfter, err := r.SolanaClient.GetTokenAccountBalance(r.Ctx, account, rpc.CommitmentConfirmed) + if err != nil { + return fmt.Errorf("failed to get balance after: %w", err) + } + + beforeAmount := utils.ParseInt(r, balanceBefore.Value.Amount) + afterAmount := utils.ParseInt(r, balanceAfter.Value.Amount) + + if isDecrease { + return require.Equal(r, beforeAmount-expectedDelta, afterAmount) + } + return require.Equal(r, beforeAmount+expectedDelta, afterAmount) +}Also applies to: 36-36, 58-58, 61-61
e2e/runner/verify.go (1)
Line range hint
107-107: Consider documenting the commitment level trade-off.The switch to
CommitmentConfirmedtrades finality for speed, which is suitable for testing. Consider adding a comment in the code or documentation explaining this trade-off and whyCommitmentConfirmedwas chosen overCommitmentFinalizedfor future maintainers.Also applies to: 122-122, 45-47
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
e2e/e2etests/test_solana_whitelist_spl.go(3 hunks)e2e/e2etests/test_spl_deposit.go(2 hunks)e2e/e2etests/test_spl_deposit_and_call.go(2 hunks)e2e/e2etests/test_spl_withdraw.go(2 hunks)e2e/e2etests/test_spl_withdraw_and_create_receiver_ata.go(2 hunks)e2e/runner/balances.go(2 hunks)e2e/runner/setup_solana.go(2 hunks)e2e/runner/solana.go(8 hunks)e2e/runner/verify.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
e2e/e2etests/test_spl_withdraw_and_create_receiver_ata.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_spl_deposit.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_spl_withdraw.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/verify.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_spl_deposit_and_call.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/e2etests/test_solana_whitelist_spl.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/balances.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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: start-solana-test / e2e
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (6)
e2e/runner/solana.go (2)
145-145: EnsureCommitmentConfirmedis sufficient for blockhash validity.Changing the commitment level to
rpc.CommitmentConfirmedmay help prevent blockhash expiration between signing and broadcasting. However, verify thatCommitmentConfirmedprovides a recent enough blockhash to avoid potential transaction failures due to expired blockhashes.
426-431: Consistent application of compute budget instructions.The addition of compute budget instructions in
SOLDepositAndCallaligns with their usage inSPLDepositAndCall. Ensure that this consistency is maintained across all transaction constructions that may benefit from compute budget optimizations.e2e/e2etests/test_solana_whitelist_spl.go (1)
70-74: Ensure account existence after whitelisting.After the whitelist transaction, confirm that the
whitelistEntryInfois not only non-nil but also contains the expected data to verify successful whitelisting.e2e/e2etests/test_spl_deposit.go (1)
26-30: Consistent use of commitment levels when fetching balances.Changing the commitment level to
rpc.CommitmentConfirmedmay result in fetching balances that are not yet finalized. Verify that this level of commitment is appropriate for your test scenarios.e2e/runner/balances.go (1)
107-107: LGTM! Consistent commitment level changes.The changes to use
rpc.CommitmentConfirmedfor both SOL and SPL token balance queries align with the PR's objective to improve transaction broadcasting reliability in e2e tests.Also applies to: 122-122
e2e/runner/verify.go (1)
45-47: LGTM! Consistent use of CommitmentConfirmed.The addition of transaction options with
CommitmentConfirmedmaintains consistency with other Solana client calls and supports the PR's goal of improving transaction reliability.
| time.Sleep(5 * time.Second) // wait a bit and check if its confirmed | ||
| blockHeight, err := r.SolanaClient.GetBlockHeight(r.Ctx, rpc.CommitmentConfirmed) | ||
| require.NoError(r, err) | ||
| r.Logger.Info("Current block height %d", blockHeight) | ||
|
|
||
| out, err = r.SolanaClient.GetTransaction(r.Ctx, sig, &rpc.GetTransactionOpts{ | ||
| Commitment: rpc.CommitmentConfirmed, | ||
| }) | ||
| if err != nil { | ||
| r.Logger.Info("Error getting tx %s", err.Error()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement a more robust confirmation mechanism.
Using time.Sleep followed by a single transaction status check may not be reliable. Consider implementing a polling mechanism with exponential backoff to check the transaction status until it is confirmed or a timeout occurs.
| // BroadcastTxSync broadcasts a transaction and waits for it to be finalized | ||
| func (r *E2ERunner) BroadcastTxSync(tx *solana.Transaction) (solana.Signature, *rpc.GetTransactionResult) { | ||
| r.Logger.Info("Broadcast start") | ||
| start := time.Now() | ||
| timeout := 2 * time.Minute // Expires after 2 mins | ||
| sig, out, isConfirmed := r.BroadcastTxSyncOnce(tx) | ||
| for { | ||
| require.False(r, time.Since(start) > timeout, "waiting solana tx timeout") | ||
| require.False(r, time.Since(start) > timeout, "solana tx timeout") | ||
|
|
||
| time.Sleep(1 * time.Second) | ||
| out, err = r.SolanaClient.GetTransaction(r.Ctx, sig, &rpc.GetTransactionOpts{}) | ||
| if err == nil { | ||
| break | ||
| if isConfirmed { | ||
| r.Logger.Info("Tx broadcasted and confirmed") | ||
| return sig, out | ||
| } | ||
| } | ||
|
|
||
| return sig, out | ||
| r.Logger.Info("Manually retrying tx") | ||
| sig, out, isConfirmed = r.BroadcastTxSyncOnce(tx) | ||
| } |
There was a problem hiding this comment.
Prevent potential duplicate transaction submissions.
Manually retrying transaction broadcasts without checking for possible duplicate submissions may lead to nonce errors or rejection by the network. Ensure that retries handle idempotency and do not unintentionally resubmit the same transaction.
| r.Logger.Info("Broadcast once start") | ||
| maxRetries := uint(1) | ||
| sig, err := r.SolanaClient.SendTransactionWithOpts(r.Ctx, tx, rpc.TransactionOpts{ | ||
| SkipPreflight: true, | ||
| MaxRetries: &maxRetries, | ||
| PreflightCommitment: rpc.CommitmentConfirmed, | ||
| }) |
There was a problem hiding this comment.
Reconsider skipping preflight checks and minimal retries.
Setting SkipPreflight to true and MaxRetries to 1 may increase the risk of transaction failures due to unanticipated issues. Preflight checks help catch errors before submission, and allowing more retries can improve reliability.
Apply this diff to re-enable preflight checks and increase retries:
sig, err := r.SolanaClient.SendTransactionWithOpts(r.Ctx, tx, rpc.TransactionOpts{
- SkipPreflight: true,
- MaxRetries: &maxRetries,
+ SkipPreflight: false,
+ MaxRetries: nil, // Use default retry behavior
PreflightCommitment: rpc.CommitmentConfirmed,
})📝 Committable 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.
| r.Logger.Info("Broadcast once start") | |
| maxRetries := uint(1) | |
| sig, err := r.SolanaClient.SendTransactionWithOpts(r.Ctx, tx, rpc.TransactionOpts{ | |
| SkipPreflight: true, | |
| MaxRetries: &maxRetries, | |
| PreflightCommitment: rpc.CommitmentConfirmed, | |
| }) | |
| r.Logger.Info("Broadcast once start") | |
| sig, err := r.SolanaClient.SendTransactionWithOpts(r.Ctx, tx, rpc.TransactionOpts{ | |
| SkipPreflight: false, | |
| MaxRetries: nil, // Use default retry behavior | |
| PreflightCommitment: rpc.CommitmentConfirmed, | |
| }) |
| info, _ := r.SolanaClient.GetAccountInfoWithOpts( | ||
| r.Ctx, | ||
| pdaAta, | ||
| &rpc.GetAccountInfoOpts{Commitment: rpc.CommitmentConfirmed}, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors when account info is unavailable.
The current implementation ignores the error returned by GetAccountInfoWithOpts. Consider checking and handling this error to ensure that unexpected issues are properly managed.
Apply this diff to handle the error:
info, err := r.SolanaClient.GetAccountInfoWithOpts(
r.Ctx,
pdaAta,
&rpc.GetAccountInfoOpts{Commitment: rpc.CommitmentConfirmed},
)
+if err != nil {
+ // Handle the error appropriately
+ require.NoError(r, err)
+}Committable suggestion skipped: line range outside the PR's diff.
| whitelistEntryInfo, err := r.SolanaClient.GetAccountInfoWithOpts( | ||
| r.Ctx, | ||
| whitelistEntryPDA, | ||
| &rpc.GetAccountInfoOpts{Commitment: rpc.CommitmentConfirmed}, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors returned by GetAccountInfoWithOpts.
Currently, the code expects an error when fetching account info for a non-existent whitelist entry but does not explicitly handle unexpected errors. Enhance error handling to distinguish between expected and unexpected errors.
Apply this diff to improve error handling:
whitelistEntryInfo, err := r.SolanaClient.GetAccountInfoWithOpts(
r.Ctx,
whitelistEntryPDA,
&rpc.GetAccountInfoOpts{Commitment: rpc.CommitmentConfirmed},
)
-require.Error(r, err)
+if err == nil {
+ require.Nil(r, whitelistEntryInfo, "Whitelist entry should not exist before whitelisting")
+} else {
+ // Handle other potential errors
+ require.Contains(r, err.Error(), "account not found", "Unexpected error: %v", err)
+}📝 Committable 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.
| whitelistEntryInfo, err := r.SolanaClient.GetAccountInfoWithOpts( | |
| r.Ctx, | |
| whitelistEntryPDA, | |
| &rpc.GetAccountInfoOpts{Commitment: rpc.CommitmentConfirmed}, | |
| ) | |
| whitelistEntryInfo, err := r.SolanaClient.GetAccountInfoWithOpts( | |
| r.Ctx, | |
| whitelistEntryPDA, | |
| &rpc.GetAccountInfoOpts{Commitment: rpc.CommitmentConfirmed}, | |
| ) | |
| if err == nil { | |
| require.Nil(r, whitelistEntryInfo, "Whitelist entry should not exist before whitelisting") | |
| } else { | |
| // Handle other potential errors | |
| require.Contains(r, err.Error(), "account not found", "Unexpected error: %v", err) | |
| } |
| receiverAtaAcc, err := r.SolanaClient.GetAccountInfoWithOpts( | ||
| r.Ctx, | ||
| receiverAta, | ||
| &rpc.GetAccountInfoOpts{Commitment: rpc.CommitmentConfirmed}, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance account existence verification
The current implementation could benefit from more specific error handling and clearer account existence checks.
+func isAccountExists(client *rpc.Client, ctx context.Context, account solana.PublicKey) (bool, error) {
+ info, err := client.GetAccountInfoWithOpts(ctx, account, &rpc.GetAccountInfoOpts{
+ Commitment: rpc.CommitmentConfirmed,
+ })
+ if err != nil {
+ if strings.Contains(err.Error(), "could not find account") {
+ return false, nil
+ }
+ return false, fmt.Errorf("failed to get account info: %w", err)
+ }
+ return info != nil, nil
+}Also applies to: 69-73
| @@ -36,7 +36,7 @@ func (r *E2ERunner) SetupSolana(gatewayID, deployerPrivateKey string) { | |||
| // get deployer account balance | |||
| privkey, err := solana.PrivateKeyFromBase58(deployerPrivateKey) | |||
| require.NoError(r, err) | |||
| bal, err := r.SolanaClient.GetBalance(r.Ctx, privkey.PublicKey(), rpc.CommitmentFinalized) | |||
| bal, err := r.SolanaClient.GetBalance(r.Ctx, privkey.PublicKey(), rpc.CommitmentConfirmed) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add initialization validation and improve error handling
The setup process should validate initialization parameters and handle errors more gracefully.
+func (r *E2ERunner) validateSolanaSetup(gatewayID string, deployerPrivateKey string) error {
+ if gatewayID == "" {
+ return errors.New("gateway ID cannot be empty")
+ }
+ if deployerPrivateKey == "" {
+ return errors.New("deployer private key cannot be empty")
+ }
+ if r.SolanaClient == nil {
+ return errors.New("solana client not initialized")
+ }
+ return nil
+}
func (r *E2ERunner) SetupSolana(gatewayID, deployerPrivateKey string) {
+ if err := r.validateSolanaSetup(gatewayID, deployerPrivateKey); err != nil {
+ r.Logger.Fatal("failed to validate setup: %v", err)
+ }Also applies to: 71-73
Description
Improve broadcasting of solana txs in e2e test. This was tested on mainnet and contains couple of fixes, I tried 5 consecutive spl deposits and all worked without tx dropping.
For context, tx was dropped on mainnet and the biggest issue is that blockhash expired sometimes between tx signing and broadcasting, therefore changed to use confirmed commitment level, which is fine, especially for tests, since this should reduce that interval.
Other fixes:
How Has This Been Tested?
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Improvements
Technical Updates
Performance
These changes aim to improve the reliability and precision of blockchain interactions in our testing and deployment processes.