feat: sol withdraw and call #3450
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 📝 WalkthroughWalkthroughThis PR introduces multiple enhancements and refactoring efforts focused on Solana integrations. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant E2ERunner
participant SolanaRunner
participant Signer
participant SolanaNode
User->>E2ERunner: Initiate SOL Withdraw and Call
E2ERunner->>SolanaRunner: Setup Solana Environment (compute PDA, initialize connected program)
SolanaRunner->>Signer: Prepare Execute Transaction (decoding, compliance checks)
Signer->>Signer: Create and sign MsgExecute (via TSS)
Signer->>SolanaNode: Submit signed transaction
SolanaNode-->>Signer: Confirm transaction mined
Signer-->>E2ERunner: Return transaction result
E2ERunner-->>User: SOL Withdraw & Call complete
Possibly related issues
Possibly related PRs
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
|
|
!!!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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3450 +/- ##
===========================================
- Coverage 65.54% 65.24% -0.31%
===========================================
Files 444 445 +1
Lines 30563 30712 +149
===========================================
+ Hits 20034 20038 +4
- Misses 9675 9820 +145
Partials 854 854
|
skosito
changed the title
| utils.RequireTxSuccessful(r, receipt, "approve") | ||
|
|
||
| // withdraw | ||
| // TODO: gas limit? |
There was a problem hiding this comment.
we should probably use gas limit field for compute budget limit
There was a problem hiding this comment.
If we use the exact budget paid by user, what will happen if user_paid_gas < gas_required_for_call? If the outbound tx fails, does it mean the nonce will not increment? Do we allow method execute to fail legally?
There was a problem hiding this comment.
good question, i am not sure, dont we have a way to revert? we didnt before but not sure if it was implemented in meantime
but there is no way afaik to still increment the nonce if it fails because of gas limit, and we have no control over how much gas it will take, so i think revert is important in this case
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
zetaclient/chains/solana/signer/whitelist.go (1)
60-67: LGTM! Consider documenting the account ordering.The account metadata ordering follows Solana's best practices with the fee payer (relayer) first. Consider adding a comment explaining the account ordering requirements to help future maintainers.
Add a comment like:
AccountValues: []*solana.AccountMeta{ + // Account ordering: + // 1. Fee payer (relayer) - requires WRITE and SIGNER + // 2. PDA - requires WRITE for state updates + // 3. Remaining accounts in order of usage solana.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER(), solana.Meta(signer.pda).WRITE(),zetaclient/chains/solana/signer/execute.go (1)
58-127: Potential optimization for blockhash retrieval
You are fetching the latest blockhash from the Solana RPC within each transaction creation. While this is correct, you could optimize throughput by caching and periodically refreshing the blockhash if multiple transactions are signed within a short timeframe. This can reduce latency and RPC overhead in high-throughput scenarios.pkg/contracts/solana/instruction.go (1)
352-407: Clear approach for encoding/decoding ExecuteMsg
The use of ABI-based unpacking followed by JSON marshalling/unmarshalling neatly bridges the structured data with your core structures, though it introduces minor overhead. Overall, it’s a valid approach unless you find performance issues at scale.e2e/runner/solana.go (2)
23-25: Consider making theConnectedProgramIDconfigurable
DefiningConnectedProgramIDas a global variable is acceptable for tests or demos, but for production, consider loading it from a configuration or environment variable for better maintainability.
477-510: NewWithdrawAndCallSOLZRC20function
This method is well-structured for bridging withdrawal and execution on Solana. However, note the “TODO” comment regarding gas limit. Finalize these parameters to avoid unexpected revert or cost overhead. Also, consider more descriptive naming than “WithdrawAndCall0” in the call.pkg/contracts/solana/gateway.go (1)
69-78:ComputeConnectedPdaAddressfunction
Computing the PDA with seed “connected” is straightforward. Consider validating the resulting address more thoroughly if used for trusted operations, and ensure it aligns with your program’s security model.e2e/e2etests/test_solana_withdraw_and_call.go (2)
55-63: Consider adding validation for the message data.The test should validate that the message data is not empty and meets any required format constraints.
msg := solanacontract.ExecuteMsg{ Accounts: []solanacontract.AccountMeta{ {PublicKey: [32]byte(connectedPda.Bytes()), IsWritable: true}, {PublicKey: [32]byte(gatewayPda.Bytes()), IsWritable: false}, {PublicKey: [32]byte(privkey.PublicKey().Bytes()), IsWritable: true}, {PublicKey: [32]byte(solana.SystemProgramID.Bytes()), IsWritable: false}, }, + // Validate message data + require.NotEmpty(r, []byte("hello"), "Message data cannot be empty") Data: []byte("hello"), }
111-112: Add more granular assertions for lamport calculations.The test should include detailed assertions to validate the exact lamport calculations and ensure precise balance splits.
-require.Equal(r, connectedPdaInfoBefore.Value.Lamports+withdrawAmount.Uint64()/2, connectedPdaInfo.Value.Lamports) -require.Equal(r, senderBefore.Value.Lamports+withdrawAmount.Uint64()/2, sender.Value.Lamports) +expectedPdaLamports := connectedPdaInfoBefore.Value.Lamports + withdrawAmount.Uint64()/2 +expectedSenderLamports := senderBefore.Value.Lamports + withdrawAmount.Uint64()/2 +require.Equal(r, expectedPdaLamports, connectedPdaInfo.Value.Lamports, "Connected PDA lamports mismatch") +require.Equal(r, expectedSenderLamports, sender.Value.Lamports, "Sender lamports mismatch") + +// Verify total lamports conservation +totalBefore := connectedPdaInfoBefore.Value.Lamports + senderBefore.Value.Lamports +totalAfter := connectedPdaInfo.Value.Lamports + sender.Value.Lamports +require.Equal(r, totalBefore+withdrawAmount.Uint64(), totalAfter, "Total lamports not conserved")zetaclient/chains/solana/signer/signer.go (1)
329-349: Add transaction simulation before signing.Consider adding a transaction simulation step before signing to validate the transaction will succeed.
// sign gateway execute message by TSS sender := ethcommon.HexToAddress(cctx.InboundParams.Sender) + +// Simulate transaction before signing +if err := signer.simulateExecuteTransaction(ctx, params, height, sender, msg.Data, remainingAccounts); err != nil { + return nil, fmt.Errorf("transaction simulation failed: %w", err) +} + msgExecute, err := signer.createAndSignMsgExecute( ctx, params, height, sender, msg.Data, remainingAccounts, cancelTx, )contrib/localnet/solana/Dockerfile (1)
8-10: LGTM! Consider optimizing the COPY commands.The new files are correctly added following the existing pattern. However, you could optimize the Dockerfile by combining related COPY commands.
Consider combining related COPY commands to reduce layers:
-COPY ./gateway.so . -COPY ./gateway-keypair.json . -COPY ./connected.so . -COPY ./connected-keypair.json . +COPY ./gateway.so ./gateway-keypair.json ./connected.so ./connected-keypair.json ./
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
contrib/localnet/solana/connected.sois excluded by!**/*.socontrib/localnet/solana/gateway.sois excluded by!**/*.sogo.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)contrib/localnet/solana/Dockerfile(1 hunks)contrib/localnet/solana/connected-keypair.json(1 hunks)contrib/localnet/solana/start-solana.sh(1 hunks)e2e/e2etests/e2etests.go(3 hunks)e2e/e2etests/test_solana_withdraw_and_call.go(1 hunks)e2e/runner/setup_solana.go(1 hunks)e2e/runner/solana.go(6 hunks)go.mod(1 hunks)pkg/contracts/solana/gateway.go(2 hunks)pkg/contracts/solana/gateway.json(35 hunks)pkg/contracts/solana/gateway_message.go(5 hunks)pkg/contracts/solana/gateway_message_test.go(3 hunks)pkg/contracts/solana/instruction.go(3 hunks)zetaclient/chains/solana/observer/outbound.go(1 hunks)zetaclient/chains/solana/signer/execute.go(1 hunks)zetaclient/chains/solana/signer/signer.go(3 hunks)zetaclient/chains/solana/signer/whitelist.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- contrib/localnet/solana/connected-keypair.json
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.sh`: Review the shell scripts, point out issues relati...
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
contrib/localnet/solana/start-solana.sh
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/local.gozetaclient/chains/solana/signer/whitelist.gozetaclient/chains/solana/observer/outbound.gozetaclient/chains/solana/signer/signer.goe2e/e2etests/test_solana_withdraw_and_call.gopkg/contracts/solana/gateway.gopkg/contracts/solana/gateway_message_test.gozetaclient/chains/solana/signer/execute.goe2e/runner/setup_solana.gopkg/contracts/solana/instruction.goe2e/runner/solana.gopkg/contracts/solana/gateway_message.goe2e/e2etests/e2etests.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: start-solana-test / e2e
- GitHub Check: start-e2e-test / e2e
- GitHub Check: build-and-test
🔇 Additional comments (46)
go.mod (1)
314-314: Dependency Version Update forprotocol-contracts-solana/go-idlThe dependency version has been updated from
v0.0.0-20241108171442-e48d82f94892tov0.0.0-20250210125843-0384ef07ec07. This change aligns with the new Solana contract features introduced in this PR. Please ensure that any breaking changes or API modifications associated with this dependency update are fully validated through your planned integration and unit tests.zetaclient/chains/solana/signer/whitelist.go (1)
78-82: Consider implementing compute budget settings.The TODO comment highlights an important optimization for transaction reliability and prioritization. Setting compute limits and prices can help prevent unexpected failures and ensure timely transaction processing.
Let's check if issue #2599 is still open and relevant:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check the status of the compute budget issue gh issue view 2599 --json state,title,bodyLength of output: 769
Confirmation: Compute Budget Settings Still Relevant
The referenced issue (#2599) remains open, confirming that the explore-and-optimize compute budget settings (compute unit limit and price) are still pending. This validates the TODO comment in the code, and it would be prudent to track and evaluate the implementation of these settings to improve transaction reliability and prioritization.
- Issue Explorer Solana priority fee and compute budget for outbound. #2599 is open: The GitHub query indicates that the issue is still active.
- Optimization suggestion remains valid: Implementing compute budget controls could prevent unexpected failures and help manage transaction fees more dynamically.
zetaclient/chains/solana/signer/execute.go (4)
16-25: Well-structured function signature for message creation and signing
This function logically collects the required parameters for message execution, correctly returning both the message and any errors.
33-35: Good approach to handlingcancelTx
Zeroing out the amount upon cancellation ensures that the withdrawal portion is neutralized when needed. This is straightforward and clear.
37-41: Proper error handling when decoding the Solana wallet address
Wrapping the error with context helps pinpoint the root cause if decoding fails. This is highly commendable for maintainability and debugging.
47-52: Appropriate TSS usage for message signing
Using the TSS component for signing the message is properly integrated, with an explanatory error wrapping. This helps ensure secure signature generation.pkg/contracts/solana/instruction.go (4)
147-148: Error reporting improved
Including both the actual and expected discriminators in the error message is helpful for troubleshooting. This is a clear, user-friendly improvement.
153-180: Comprehensive struct definition for ExecuteInstructionParams
All essential fields required for execution, including signatures and the message hash, are well-defined. This structure aligns with existing patterns for other Solana instructions.
201-216: Robust parse function for execute instructions
Decoding via Borsh and validating the discriminator ensure that only legitimate execute instructions pass through. The error wrapping provides clarity for debugging.
347-350: Minimal struct for account metadata
Storing the public key and writable flag as foundational data respects Solana’s model. The size constraints (32 bytes) align well with Solana’s key format.pkg/contracts/solana/gateway_message.go (5)
11-16: Consistent use of instruction byte codes
Applying distinct byte codes for each instruction enhances clarity and maintainability. This uniform scheme will help prevent collisions and confusion.
71-72: Clear adoption of "ZETACHAIN" prefix
Using a unified prefix fosters consistency across different messages and strengthens the identity of these signed messages.
119-144: Concise struct definition for MsgExecute
All fields needed for an execution flow appear logically grouped and named. This fosters readability and straightforward usage in the rest of the codebase.
334-335: Well-aligned hash prefix for SPL withdraw
Switching to "ZETACHAIN" plus a standardized byte code is consistent with the improvements seen in other instructions and supports better uniformity.
440-441: Uniform design for whitelist instruction
Adhering to the standardized prefix and byte code approach is consistent and logical, ensuring the same pattern applies across all instructions.e2e/runner/solana.go (6)
17-17: Import usage appears consistent
No issues spotted with importinggit.luolix.top/zeta-chain/protocol-contracts/pkg/gatewayzevm.sol.
32-32: Log statement is clear
The updated log message offers better context. This change is concise and maintains clarity.
87-88: Confirm account meta ordering
Revising the order in account metadata should align with the gateway’s expectations. Confirm that these positions match the Solana program’s instruction schema.
243-245: Higher compute unit limit
Raising the limit from 50,000 to 70,000 increases the upper bound on compute utilization, which can be useful but might incur higher costs. Ensure it’s truly needed.
453-453: Approval logic appears correct
The approval step properly precedes withdrawal. The subsequent checks and receipt validation help ensure correctness.
459-465: Withdrawal with revert options
AddingRevertOptions{OnRevertGasLimit: big.NewInt(0)}is a helpful measure to control revert behavior. The usage looks correct, but be sure to confirm the targeted revert logic in your gateway contract.pkg/contracts/solana/gateway_message_test.go (3)
23-23: Updated hash forTest_MsgWithdrawHash
The new hash value reflects updated logic. Ensure the changed reference is in sync with any modifications in the message structure or hashing function.
44-44: Updated hash forTest_MsgWhitelistHash
The updated expected hash must match new or revised data fields in the whitelist message. This change looks correct if the struct or hashing mechanism changed accordingly.
68-68: Updated hash forTest_MsgWithdrawSPLHash
Changes to the SPL withdraw logic or message fields likely necessitated this new hash. Confirm it is consistent with the updated message format.pkg/contracts/solana/gateway.go (1)
42-44: Execute discriminator introduced
This newDiscriminatorExecuteis clear in purpose and naming. Ensuring test coverage for theexecuteinstruction would further validate its correctness.cmd/zetae2e/local/local.go (1)
405-405: LGTM!The new test case is correctly added to the
solanaTestsslice, maintaining consistent grouping with other Solana-related tests.e2e/e2etests/e2etests.go (3)
52-52: LGTM!The constant name follows the established naming convention and is appropriately placed in the Solana tests section.
457-464: LGTM!The test definition is well-structured with:
- Clear description
- Consistent default value for amount (1,000,000 lamports)
- Proper function reference
445-445: LGTM!The default value update for Solana deposit test (24,000,000 lamports) aligns with the test requirements.
pkg/contracts/solana/gateway.json (16)
7-7: Metadata Description Update:
The"description"field in the metadata section has been updated to"ZetaChain Gateway program on Solana", which clearly identifies the program’s purpose on Solana. This is an effective improvement in documentation.
12-19: Deposit Instruction Documentation Enhancements:
Thedepositinstruction has received comprehensive documentation updates. The added"docs"entries (lines 12–19) explain the arguments and context, and the associated accounts (signer,pda, andsystem_program) also now include clear descriptions (lines 33–35, 41–43, and 61–63 respectively). These changes enhance clarity and guide users on what each parameter and account represents.Also applies to: 33-35, 41-43, 61-63
85-93: Deposit-and-Call Instruction Improvement:
Thedeposit_and_callinstruction now includes detailed documentation that explains the deposit process as well as the mechanism for invoking a contract call (lines 85–93). In addition, the account descriptions for thesigner,pda, andsystem_program(lines 107–109, 115–117, and 135–137) have been updated. This improves overall consistency across instructions.Also applies to: 107-109, 115-117, 135-137
163-170: SPL Token Deposit Instruction Updates:
Thedeposit_spl_tokeninstruction has been enriched with a comprehensive docs array (lines 163–170) that explains the handling of SPL token deposits and their relation to the ZetaChain zEVM. Furthermore, documentation for the related accounts—signer(lines 183–186),pda(191–194),whitelist_entry(211–214),mint_account(239–242),token_program(245–249),from(254–256),to(259–264), andsystem_program(266–270)—has been added. This level of detail is useful for developers and auditors alike.Also applies to: 183-186, 191-194, 211-214, 239-242, 245-249, 254-256, 259-264, 266-270
290-299: SPL Token Deposit-and-Call Instruction Documentation:
Similar to the SOL deposit instructions, thedeposit_spl_token_and_callinstruction now has detailed documentation (lines 290–299) that covers both deposit and subsequent call operations. The associated accounts (signerat 312–315,pdaat 320–324,whitelist_entryat 340–344,mint_accountat 368–372,token_programat 374–378,fromat 381–386,toat 388–392, andsystem_programat 395–399) now include descriptive"docs"fields, ensuring consistency and clarity regarding the role of each account.Also applies to: 312-315, 320-324, 340-344, 368-372, 374-378, 381-386, 388-392, 395-399
423-436: New Execute Instruction Additions:
A new instruction,execute, has been introduced. The docs (lines 423–436) clearly describe how the instruction withdraws SOL and invokes theon_callmethod, including detailed explanations of all arguments required for TSS signature verification. The account documentation forsigner(448–455),pda(456–474), anddestination_program(477–481) is also well detailed. The comprehensive args block (lines 486–530) correctly defines new fields such assignature,recovery_id,message_hash, andnonce. This new instruction is documented clearly and aligns well with the intended new functionality.Also applies to: 448-455, 456-474, 477-481, 486-530
534-541: Initialize Instruction Refinements:
Theinitializeinstruction now includes updated documentation (lines 534–541) that explains initialization parameters such as the TSS address and chain ID. Additionally, account descriptions forsigner(554–560),pda(562–579), andsystem_program(582–587) have been refreshed for clarity. These changes help ensure that the initialization process is clear to users and integrators.Also applies to: 554-560, 562-579, 582-587
607-613: Set Deposit Paused Instruction Update:
Theset_deposit_pausedinstruction now documents its purpose and the boolean flag for pausing deposits (lines 607–613). The updated account documentation for bothsigner(625–632) andpda(634–652) is precise and contributes to overall consistency across the file.Also applies to: 625-632, 634-652
663-672: Unwhitelist SPL Mint Instruction Enhancements:
In theunwhitelist_spl_mintinstruction, the newly added documentation (lines 663–672) explains the process of unwhitelisting an SPL token. The description for the relevant accounts—authority(685–691),pda(692–711),whitelist_entry(713–719), andwhitelist_candidate(742–746)—has been streamlined to clarify responsibilities. These improvements facilitate better understanding of the unwhitelisting mechanism.Also applies to: 685-691, 692-711, 713-719, 742-746
779-785: Update Authority Instruction Improvements:
Theupdate_authorityinstruction now provides clear documentation (lines 779–785) for changing the PDA’s authority. The updated account descriptions forsigner(797–804) andpda(806–824) ensure that the process is well understood. This is an important change for maintaining correct authority over the gateway.Also applies to: 797-804, 806-824
835-841: Update TSS Instruction Documentation:
Theupdate_tssinstruction features a refreshed docs block (lines 835–841) that specifies the update of the TSS address. The corresponding accounts (signeron lines 853–860 andpdaon lines 862–880) have been annotated, making the operation unambiguous. This is critical for correct signature verification in cross-chain operations.Also applies to: 853-860, 862-880
896-905: Whitelist SPL Mint Instruction Updates:
Thewhitelist_spl_mintinstruction is comprehensively documented (lines 896–905), and the account details forauthority(918–924),pda(926–944),whitelist_entry(946–973),whitelist_candidate(975–979), andsystem_program(981–986) have been updated. These changes enhance clarity in the process of whitelisting SPL tokens and ensure that all roles are well described.Also applies to: 918-924, 926-944, 946-973, 975-979, 981-986
1018-1029: Withdraw Instruction Enhancements:
Thewithdrawinstruction now includes improved documentation (lines 1018–1029) describing the withdrawal of SOL, along with updated account descriptions forsigner(1042–1048),pda(1050–1068), andrecipient(1070–1075). This additional clarity helps prevent ambiguity about the withdrawal process.Also applies to: 1042-1048, 1050-1068, 1070-1075
1111-1123: Withdraw SPL Token Instruction Documentation:
Thewithdraw_spl_tokeninstruction now carries detailed documentation (lines 1111–1123) and updated account descriptions. The changes cover all relevant accounts:signer(1137–1142),pda(1145–1162),pda_ata(1165–1169),mint_account(1171–1175),recipient(1177–1181),recipient_ata(1184–1187),token_program(1191–1195),associated_token_program(1198–1202), andsystem_program(1205–1207). This thorough update ensures that the process for SPL token withdrawals is clearly documented.Also applies to: 1137-1142, 1145-1162, 1165-1169, 1171-1175, 1177-1181, 1184-1187, 1191-1195, 1198-1202, 1205-1207
1289-1327: Error Codes and Messages Reorganization:
The errors section has been reorganized with renumbered codes and updated messages. New error definitions likeInvalidInstructionDataare introduced, and redundant errors (e.g.InsufficientPoints) have been removed. This structured approach will simplify error handling and debugging.
1330-1344: Type Definitions Documentation Enhancements:
The types section has been enriched by providing detailed"docs"arrays for both thePdaandWhitelistEntrytypes. Each field (e.g.nonce,tss_address,authority, etc.) now has supporting documentation (spanning lines 1330–1390), which improves maintainability and assists developers in understanding how the structure supports program state.Also applies to: 1346-1350, 1358-1363, 1364-1369, 1371-1377, 1383-1390
changelog.md (1)
10-10: Changelog Entry for SOL Withdraw and Call Integration:
The new feature entry for [3450] is added under the features section (line 10). This entry clearly communicates the addition of SOL withdraw and call integration, aligning with the PR objectives.
Description
TODO: unit tests, will follow up on that when SPL version of this is ready
solana contract PR: https://github.com/zeta-chain/protocol-contracts-solana/pull/74/files
How Has This Been Tested?
Summary by CodeRabbit