fix: integrate withdraw spl create ata fix #3238
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 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the removal of functionalities related to the rent payer in the Solana ecosystem. Key changes include the deletion of airdrop commands in the 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
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
contrib/localnet/solana/start-solana.sh (3)
Line range hint
1-8: Add error handling for critical operations.The script performs critical operations without proper error handling, which could lead to silent failures in the setup process.
Apply this diff to add error handling:
#!/bin/bash +set -euo pipefail + +cleanup() { + echo "Cleaning up..." + pkill solana-test-validator || true +} + +trap cleanup EXIT echo "making an id" -solana-keygen new -o /root/.config/solana/id.json --no-bip39-passphrase +if ! solana-keygen new -o /root/.config/solana/id.json --no-bip39-passphrase; then + echo "Failed to generate keypair" + exit 1 +fi -solana config set --url localhost +if ! solana config set --url localhost; then + echo "Failed to set Solana config" + exit 1 +fi
Line range hint
17-18: Replace static sleep with proper process monitoring.The static sleep at the end doesn't provide meaningful debugging capabilities.
Apply this diff:
-# leave some time for debug if validator exits due to errors -sleep 1000 +echo "Setup complete. Monitoring validator..." +wait $VALIDATOR_PID
Line range hint
1-18: Consider adding logging and environment configuration.The script would benefit from better logging and configurable parameters.
Add these improvements at the beginning of the script:
#!/bin/bash + +# Configuration +LOG_FILE="solana_setup.log" +VALIDATOR_LOG="validator.log" +SOL_AMOUNT="${SOL_AMOUNT:-100}" # Amount of SOL to airdrop, configurable via env + +# Logging function +log() { + echo "[$(date '+%Y-%m-%d %H:%M:%S')] $1" | tee -a "$LOG_FILE" +} + +log "Starting Solana setup..."zetaclient/chains/solana/signer/signer.go (1)
Line range hint
106-166: Consider enhancing context handling in TryProcessOutboundThe TryProcessOutbound method could benefit from explicit context timeout handling to ensure graceful cancellation of long-running operations.
Consider adding timeout handling:
func (signer *Signer) TryProcessOutbound( ctx context.Context, cctx *types.CrossChainTx, outboundProc *outboundprocessor.Processor, outboundID string, _ interfaces.ChainObserver, zetacoreClient interfaces.ZetacoreClient, height uint64, ) { + // Add timeout to context + ctx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + // end outbound process on panic defer func() { outboundProc.EndTryProcess(outboundID)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
contrib/localnet/solana/gateway.sois excluded by!**/*.so
📒 Files selected for processing (9)
contrib/localnet/solana/start-solana.sh(1 hunks)e2e/e2etests/test_spl_withdraw_and_create_receiver_ata.go(1 hunks)e2e/runner/setup_solana.go(0 hunks)e2e/runner/solana.go(0 hunks)pkg/contracts/solana/gateway.go(0 hunks)pkg/contracts/solana/gateway.json(0 hunks)pkg/contracts/solana/instruction.go(0 hunks)zetaclient/chains/solana/signer/signer.go(1 hunks)zetaclient/chains/solana/signer/withdraw_spl.go(0 hunks)
💤 Files with no reviewable changes (6)
- pkg/contracts/solana/gateway.json
- pkg/contracts/solana/instruction.go
- e2e/runner/solana.go
- zetaclient/chains/solana/signer/withdraw_spl.go
- e2e/runner/setup_solana.go
- pkg/contracts/solana/gateway.go
✅ Files skipped from review due to trivial changes (1)
- e2e/e2etests/test_spl_withdraw_and_create_receiver_ata.go
🧰 Additional context used
📓 Path-based instructions (2)
zetaclient/chains/solana/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
contrib/localnet/solana/start-solana.sh (1)
Pattern **/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
🔇 Additional comments (2)
zetaclient/chains/solana/signer/signer.go (2)
70-73: LGTM: Clean struct initialization
The struct initialization is concise and well-organized, properly reflecting the removal of the rentPayerPda field.
Line range hint 338-352: Track and address technical debt
These placeholder functions are marked for removal in issue #2532. Consider prioritizing this cleanup to reduce technical debt.
✅ Verification successful
Let me search for these placeholder functions to verify their current state in the codebase.
Technical Debt Confirmation: Empty Interface Methods Present
The issue #2532 is still open and the codebase analysis confirms the presence of empty placeholder methods in Bitcoin, Solana, and TON signers:
- Empty
SetZetaConnectorAddressimplementations in:zetaclient/chains/bitcoin/signer/signer.gozetaclient/chains/solana/signer/signer.gozetaclient/chains/ton/signer/signer.go
- Empty
SetERC20CustodyAddressimplementations in:zetaclient/chains/bitcoin/signer/signer.gozetaclient/chains/solana/signer/signer.gozetaclient/chains/ton/signer/signer.go
These empty implementations exist solely to satisfy the interfaces.ChainSigner interface, while EVM chains maintain full implementations. Consider refactoring the interface design to avoid these no-op implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the issue is still open and relevant
gh issue view 2532 --json state,title,body
Length of output: 562
Script:
#!/bin/bash
# Check for the placeholder functions mentioned in the issue
ast-grep --pattern 'func ($_ *Signer) GetZetaConnectorAddress() common.Address {
$$$
}'
ast-grep --pattern 'func ($_ *Signer) SetZetaConnectorAddress($_) {
$$$
}'
ast-grep --pattern 'func ($_ *Signer) GetERC20CustodyAddress() common.Address {
$$$
}'
ast-grep --pattern 'func ($_ *Signer) SetERC20CustodyAddress($_) {
$$$
}'
Length of output: 3393
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3238 +/- ##
===========================================
+ Coverage 61.81% 61.82% +0.01%
===========================================
Files 431 431
Lines 30763 30751 -12
===========================================
- Hits 19015 19013 -2
+ Misses 10890 10881 -9
+ Partials 858 857 -1
|
* integrate withdraw spl create ata fix * comment
Description
integrates changes from this PR
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
These changes enhance the efficiency and clarity of the Solana integration, improving user experience during setup and operation.