feat: spl withdraw and call #3520
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 pull request integrates SPL token withdrawal and call functionality into the system. It introduces new fields and enumerations (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Signer
participant SolanaChain
participant SPLProgram
Client->>Signer: Request SPL Withdraw & Call Transaction
Signer->>Signer: Validate parameters & construct message (WithdrawAndCallSPLZRC20)
Signer->>SolanaChain: Submit signed transaction
SolanaChain->>SPLProgram: Execute SPL token instruction
SPLProgram-->>SolanaChain: Provide transaction confirmation and update balances
SolanaChain-->>Signer: Transaction confirmation details
Signer-->>Client: Return transaction status and details
sequenceDiagram
participant OutboundObserver
participant Contracts
participant Signer
OutboundObserver->>Contracts: ParseInstructionWithdrawSPL for ERC20 transaction
alt Parsing Fails
Contracts->>Contracts: Execute ParseInstructionExecuteSPL alternative
end
Contracts-->>OutboundObserver: Return parsed instruction object
OutboundObserver-->>Signer: Forward instruction for processing
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 #3520 +/- ##
===========================================
- Coverage 65.22% 64.83% -0.40%
===========================================
Files 453 454 +1
Lines 30967 31154 +187
===========================================
Hits 20199 20199
- Misses 9903 10090 +187
Partials 865 865
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
zetaclient/chains/solana/signer/execute_spl.go (2)
16-27: Clarify function documentation regarding thecancelTxparameter.
While the docstring states this function creates and signs an SPL message, it would be beneficial to explicitly mention thatcancelTxzeroes out the withdrawal amount to abort the transaction.
34-37: Consider introducing an informational log whencancelTxis triggered.
Currently, ifcancelTxis true, the amount is silently zeroed. Logging or explicitly returning a message could help troubleshoot or audit canceled transactions.pkg/contracts/solana/gateway_message.go (1)
16-16: Extend testing coverage for the new instruction identifier.
Adding a unit test that ensuresInstructionExecuteSPLis handled correctly can prevent future regressions.e2e/e2etests/test_solana_withdraw_and_call.go (1)
48-58: Leverage parameterization for test data.
Currently, the test hardcodes "hello" inWithdrawAndCallSOLZRC20. To broaden coverage, passing various messages (e.g., large or non-ASCII) would validate different edge cases.e2e/e2etests/test_spl_withdraw_and_call.go (3)
33-34: Consider improving error message for insufficient balance.The error message could be more descriptive to help with debugging.
-require.Equal(r, 1, zrc20BalanceBefore.Cmp(withdrawAmount), "Insufficient balance for withdrawal") +require.Equal(r, 1, zrc20BalanceBefore.Cmp(withdrawAmount), + "Insufficient balance for withdrawal: have %v, need %v", zrc20BalanceBefore, withdrawAmount)
35-43: Consider using constants for magic numbers.The approval amount of 1 SOL could be defined as a constant at the package level.
+const ( + // MaxApprovedWithdrawalAmount represents the maximum amount that can be withdrawn (1 SOL) + MaxApprovedWithdrawalAmount = solana.LAMPORTS_PER_SOL +) -approvedAmount := new(big.Int).SetUint64(solana.LAMPORTS_PER_SOL) +approvedAmount := new(big.Int).SetUint64(MaxApprovedWithdrawalAmount)
83-90: Consider using a dedicated package for Solana-specific types.The
ConnectedPdaInfostruct could be moved to a dedicated package for better organization.Consider moving this struct to
pkg/contracts/solana/types.go.e2e/runner/setup_solana.go (1)
97-122: Consider extracting common initialization logic.The initialization code for connected and connected SPL programs follows the same pattern. Consider extracting the common logic into a helper function.
+func (r *E2ERunner) initializeProgram( + privkey solana.PrivateKey, + programID solana.PublicKey, + pda solana.PublicKey, + discriminator [8]byte, +) error { + var inst solana.GenericInstruction + accountSlice := []*solana.AccountMeta{ + solana.Meta(privkey.PublicKey()).WRITE().SIGNER(), + solana.Meta(pda).WRITE(), + solana.Meta(solana.SystemProgramID), + } + inst.ProgID = programID + inst.AccountValues = accountSlice + + type InitializeParams struct { + Discriminator [8]byte + } + var err error + inst.DataBytes, err = borsh.Serialize(InitializeParams{ + Discriminator: discriminator, + }) + if err != nil { + return err + } + + signedTx := r.CreateSignedTransaction([]solana.Instruction{&inst}, privkey, []solana.PrivateKey{}) + _, out := r.BroadcastTxSync(signedTx) + r.Logger.Info("initialize program logs: %v", out.Meta.LogMessages) + return nil +}pkg/contracts/solana/instruction.go (1)
283-310: Fix incorrect comment for the amount field.The comment for the amount field incorrectly refers to "withdraw" instead of "execute".
- // Amount is the lamports amount for the withdraw + // Amount is the lamports amount for the executezetaclient/chains/solana/signer/signer.go (1)
412-482: Enhance error handling with wrapped errors.The error handling could be improved by wrapping errors with context using
errors.Wraporerrors.Wrapf, similar to other functions in the codebase.// get mint details to get decimals mint, err := signer.decodeMintAccountDetails(ctx, cctx.InboundParams.Asset) if err != nil { - return nil, err + return nil, errors.Wrap(err, "decodeMintAccountDetails error") } message, err := hex.DecodeString(cctx.RelayedMessage) if err != nil { - return nil, err + return nil, errors.Wrapf(err, "decodeString %s error", cctx.RelayedMessage) } msg, err := contracts.DecodeExecuteMsg(message) if err != nil { - return nil, err + return nil, errors.Wrapf(err, "decodeExecuteMsg %s error", cctx.RelayedMessage) }e2e/runner/solana.go (2)
602-602: Address TODO comment about gas limit.The gas limit calculation needs to be determined and implemented.
Would you like me to help determine an appropriate gas limit calculation for this operation?
24-25: Consider making program IDs configurable.Hard-coded program IDs should be moved to configuration to support different environments.
Consider:
- Moving these to a configuration struct
- Passing them through constructor or setup methods
- Supporting environment-specific values
changelog.md (1)
13-13: Fix markdown formatting for URL.The URL should be properly formatted as a markdown link.
Apply this diff to fix the markdown formatting:
-* [3520] (https://github.com/zeta-chain/node/pull/3520) - SPL withdraw and call integration +* [3520](https://github.com/zeta-chain/node/pull/3520) - SPL withdraw and call integration🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
13-13: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
contrib/localnet/solana/connected_spl.sois excluded by!**/*.socontrib/localnet/solana/gateway.sois excluded by!**/*.sogo.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)contrib/localnet/solana/Dockerfile(0 hunks)contrib/localnet/solana/connected_spl-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(2 hunks)e2e/e2etests/test_spl_withdraw_and_call.go(1 hunks)e2e/runner/setup_solana.go(1 hunks)e2e/runner/solana.go(2 hunks)go.mod(1 hunks)pkg/contracts/solana/gateway.go(2 hunks)pkg/contracts/solana/gateway.json(1 hunks)pkg/contracts/solana/gateway_message.go(3 hunks)pkg/contracts/solana/instruction.go(2 hunks)zetaclient/chains/solana/observer/outbound.go(1 hunks)zetaclient/chains/solana/signer/execute_spl.go(1 hunks)zetaclient/chains/solana/signer/signer.go(2 hunks)
💤 Files with no reviewable changes (1)
- contrib/localnet/solana/Dockerfile
✅ Files skipped from review due to trivial changes (1)
- contrib/localnet/solana/connected_spl-keypair.json
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.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.
zetaclient/chains/solana/observer/outbound.gocmd/zetae2e/local/local.goe2e/e2etests/test_solana_withdraw_and_call.goe2e/runner/solana.goe2e/e2etests/test_spl_withdraw_and_call.goe2e/runner/setup_solana.gozetaclient/chains/solana/signer/signer.goe2e/e2etests/e2etests.gopkg/contracts/solana/instruction.gozetaclient/chains/solana/signer/execute_spl.gopkg/contracts/solana/gateway.gopkg/contracts/solana/gateway_message.go
`**/*.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
🪛 markdownlint-cli2 (0.17.2)
changelog.md
13-13: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-zetanode
- GitHub Check: lint
- GitHub Check: build-and-test
- GitHub Check: gosec
- GitHub Check: build
- GitHub Check: Analyze (go)
🔇 Additional comments (17)
zetaclient/chains/solana/signer/execute_spl.go (2)
79-81: Use consistent casing for the key-sign error message.
A previous review recommended adjusting the message to lowercase for stylistic consistency.
88-171: Confirm the necessity of including all token attributes in the transaction hash.
The current approach excludes certain fields (e.g., the relayer's or TSS's public key) from the message hash. Verify if this is intentional to preserve the correct signer context and prevent replay attacks.pkg/contracts/solana/gateway_message.go (1)
222-223: Validate critical fields inMsgExecute's hashing logic.
It omits the sender from the hash, which might permit replay if the sender is not otherwise validated. Ensure this aligns with your security model.pkg/contracts/solana/gateway.go (3)
45-46: LGTM!The new discriminator constant for SPL token execution is well-documented and follows the established pattern.
72-80: LGTM!The
ComputePdaAddressfunction provides a flexible and reusable way to compute PDA addresses with custom seeds.
82-85: LGTM!The refactored functions
ComputeConnectedPdaAddressandComputeConnectedSPLPdaAddresseffectively utilize the new genericComputePdaAddressfunction, promoting code reuse.Also applies to: 87-90
e2e/e2etests/test_spl_withdraw_and_call.go (1)
19-23: LGTM!The function documentation clearly explains the test's purpose and expected behavior.
zetaclient/chains/solana/observer/outbound.go (1)
316-321: LGTM!The error handling for SPL token instructions follows the same pattern as gas token instructions, maintaining consistency in the codebase.
pkg/contracts/solana/instruction.go (1)
219-242: LGTM! Well-structured instruction parameters.The struct follows the established pattern and correctly implements the OutboundInstruction interface.
cmd/zetae2e/local/local.go (1)
416-416: LGTM! Test case addition.The new test case follows the established pattern for Solana tests.
e2e/runner/solana.go (1)
558-621: LGTM! Well-structured implementation.The function follows the established pattern and correctly handles token approvals, message encoding, and transaction execution.
e2e/e2etests/e2etests.go (1)
61-61: LGTM! New test constant follows naming convention.The new test constant
TestSPLWithdrawAndCallNameis well-defined and follows the established naming pattern.contrib/localnet/solana/start-solana.sh (1)
16-16: LGTM! Program deployment follows existing pattern.The deployment of
connected_spl.sois correctly placed after related program deployments.go.mod (1)
314-314:✅ Verification successful
Verify compatibility with the new go-idl version.
The update to
protocol-contracts-solana/go-idl(v0.0.0-20250211174435-9680e27af84a) supports the new SPL functionality. Ensure all dependent packages are compatible with this version.
🏁 Script executed:
#!/bin/bash # Check for any version conflicts or breaking changes go mod tidy go mod verifyLength of output: 23918
Compatibility Verified: Updated go-idl Dependency
The update to
github.com/zeta-chain/protocol-contracts-solana/go-idl(v0.0.0-20250211174435-9680e27af84a) has been validated for compatibility. Runninggo mod tidyandgo mod verifyproduced no conflicts or errors, confirming that all dependent packages are compatible with the new SPL functionality support.pkg/contracts/solana/gateway.json (2)
533-687: LGTM! Well-structured SPL token execution instruction.The
execute_spl_tokeninstruction is well-documented and follows the established pattern:
- Comprehensive documentation with clear argument descriptions
- Proper account definitions with appropriate write permissions
- Unique discriminator
- Complete set of required arguments including security parameters
588-593:❓ Verification inconclusive
Verify ATA account initialization.
Ensure that the
pda_ataanddestination_program_pda_ataaccounts are properly initialized before token transfers. Consider adding a check in the instruction to verify the accounts exist.Also applies to: 610-615
🏁 Script executed:
#!/bin/bash # Search for ATA initialization patterns in the codebase ast-grep --pattern 'create_associated_token_account($$$)'Length of output: 59
Action Required: Confirm ATA Account Initialization
Please verify that both the
pda_ataanddestination_program_pda_ataaccounts are fully initialized prior to any token transfer operations. Our initial check using the AST pattern forcreate_associated_token_account($$$)did not return any evidence of such initialization. This uncertainty applies to the asset definitions in the JSON (lines 588–593 and 610–615).
- Manually confirm whether the associated token account creation is handled elsewhere, or if explicit checks (or instruction logic) are required to ensure the accounts exist before token transfers.
- If initialization is handled in a different part of the codebase (or via off-chain flows), add a comment or documentation reference to clarify this process.
- Consider updating the logic to include an explicit check if no prior initialization guarantees exist.
changelog.md (1)
1-892: LGTM! Well-structured changelog.The changelog follows best practices with:
- Clear version sections
- Organized subsections (Features, Fixes, Refactors, etc.)
- PR links and descriptions
- Breaking changes clearly highlighted
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
13-13: Bare URL used
null(MD034, no-bare-urls)
162-162: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
166-166: Multiple headings with the same content
null(MD024, no-duplicate-heading)
168-168: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
174-174: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
176-176: Multiple headings with the same content
null(MD024, no-duplicate-heading)
178-178: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
179-179: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
183-183: Multiple headings with the same content
null(MD024, no-duplicate-heading)
185-185: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
189-189: Multiple headings with the same content
null(MD024, no-duplicate-heading)
191-191: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
599-599: Multiple headings with the same content
null(MD024, no-duplicate-heading)
601-601: Bare URL used
null(MD034, no-bare-urls)
646-646: Multiple headings with the same content
null(MD024, no-duplicate-heading)
702-702: Multiple headings with the same content
null(MD024, no-duplicate-heading)
|
@brewmaster012 i will merge this PR just so its not long lived, but if you spot something we can just fix it afterwards, please let me know also, most of the important points we are already discussing on solana repo PRs, so this is ok to merge imo |
Description
solana PR: zeta-chain/protocol-contracts-solana#77
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor & Tests