fix: solana call required accounts number condition#3895
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 change updates the Solana gateway logic and tests to refine the handling and validation of account counts for call instructions. It introduces a new constant for required accounts, adjusts the validation logic to require at least one signer account, and updates related tests and documentation for clarity and consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant Validator
Client->>Gateway: Submit Solana call instruction
Gateway->>Gateway: Validate signer accounts (require at least 1)
alt If valid
Gateway->>Validator: Process instruction
Validator-->>Gateway: Return result
Gateway-->>Client: Return success
else If invalid
Gateway-->>Client: Return error (wrong signer accounts count)
end
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (1)
pkg/contracts/solana/gateway.go (1)
14-25: Improve naming consistency across constants.The constants use inconsistent naming patterns. Consider standardizing to either all exported or all unexported with a consistent naming scheme.
Apply consistent naming pattern:
-// accountsNumDeposit is the number of accounts required for Solana gateway deposit instruction -// [signer, pda, system_program] -accountsNumDeposit = 3 - -// accountsNumberDepositSPL is the number of accounts required for Solana gateway deposit spl instruction -// [signer, pda, whitelist_entry, mint_account, token_program, from, to, system_program] -accountsNumberDepositSPL = 8 - -// accountsNumberCall is the number of accounts required for Solana gateway call instruction -// [signer] -accountsNumberCall = 1 +// accountsNumDeposit is the number of accounts required for Solana gateway deposit instruction +// [signer, pda, system_program] +accountsNumDeposit = 3 + +// accountsNumDepositSPL is the number of accounts required for Solana gateway deposit spl instruction +// [signer, pda, whitelist_entry, mint_account, token_program, from, to, system_program] +accountsNumDepositSPL = 8 + +// accountsNumCall is the number of accounts required for Solana gateway call instruction +// [signer] +accountsNumCall = 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
changelog.md(1 hunks)pkg/contracts/solana/gateway.go(1 hunks)pkg/contracts/solana/inbound.go(1 hunks)pkg/contracts/solana/inbound_test.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/contracts/solana/gateway.gopkg/contracts/solana/inbound.gopkg/contracts/solana/inbound_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: start-solana-test / e2e
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (4)
pkg/contracts/solana/gateway.go (1)
22-25: Well-documented constant addition.The new
accountsNumberCallconstant is properly documented and aligns with the PR objective of standardizing account validation for call instructions.changelog.md (1)
52-52: Clear and concise changelog entry.The changelog entry accurately documents the fix and follows the established format with proper PR reference.
pkg/contracts/solana/inbound.go (1)
248-250:Details
✅ Verification successful
Validation logic updated appropriately.
The change from exact count validation to minimum count validation aligns with the PR objective. The updated error message clearly indicates the requirement for at least one signer account, which is appropriate for call instructions.
Verify that relaxing the validation from exact count to minimum count doesn't introduce security vulnerabilities:
🏁 Script executed:
#!/bin/bash # Search for other usages of ParseInboundAsCall to ensure this change is safe rg -A 10 -B 5 "ParseInboundAsCall"Length of output: 6489
✅ Validation logic update approved—ensure no unintended relaxation
The change from an exact-to-a-minimum account count (
if len(instructionAccounts) < accountsNumberCall) correctly enforces “at least one signer” and preserves existing tests (which still error when there are zero signers). I reviewed all call‐parsing sites—only the Solana observer invokes ParseInboundAsCall—and there are no other call‐count invariants in the codebase.No further fixes required, but please confirm in your integration tests that no instruction ever provides extra signer accounts that could subvert downstream logic.
pkg/contracts/solana/inbound_test.go (1)
113-114: Test simplification improves clarity.The consistent pattern of simplifying test setup across all "wrong accounts count" test cases improves readability and maintainability. Removing the complexity of adding extra account keys while maintaining error condition validation is a good refactoring approach.
Also applies to: 200-201, 285-286, 373-374, 458-459
3 participants
Description
https://github.com/zeta-chain/protocol-private/issues/263
How Has This Been Tested?
Summary by CodeRabbit
Bug Fixes
Documentation
Tests