refactor: cli command to fetch inbound ballot from inbound hash and move to zetatools [WIP] #3368
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 introduces a comprehensive refactoring of the command-line interface (CLI) for fetching inbound ballot transactions. The changes involve relocating and restructuring the inbound ballot functionality from Changes
Sequence DiagramsequenceDiagram
participant CLI as ZetaTool CLI
participant Config as Configuration
participant Client as ZetaCore Client
participant Chain as Blockchain Network
CLI->>Config: Retrieve network configuration
Config-->>CLI: Return configuration
CLI->>Client: Initialize ZetaCore client
Client-->>CLI: Client ready
CLI->>Chain: Fetch transaction details
Chain-->>CLI: Return transaction information
CLI->>CLI: Process and identify ballot
CLI->>CLI: Return ballot identifier
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
The refactoring presents a more modular and extensible approach to handling inbound ballot transactions, improving the overall CLI experience and maintainability of the codebase. 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
|
kingpinXD
changed the title
|
!!!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 |
kingpinXD
changed the title
kingpinXD
changed the title
| return identifierFromBtcEvent(event, senderChainID, zetacoreChainID) | ||
| } | ||
|
|
||
| func identifierFromBtcEvent(event *zetaclientObserver.BTCInboundEvent, |
There was a problem hiding this comment.
This follows similar logic to as processing Bitcoin Inbound Trackers , we can consider refactoring the tracker logic in the future so that it can be reused .
Similar comments for EVM
Solana logic could be used as it needed only minor changes on the zetaclient side
There was a problem hiding this comment.
The solution also bypasses som checks such as
- Checking for restricted address
- Checking for Donation message (IsEventProcessable check)
I don't think those are required for a debugging tool, but they can be added if needed
We should consider though to refactor the client logic so derive out the common logic from this GetBallotTool , ProcessInboundTracker and in some cases ProcessInbound
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
zetaclient/chains/solana/observer/inbound.go (1)
Line range hint
233-244: Extract common event creation logic to reduce duplication.The event creation logic is duplicated between SOL and SPL deposits. Consider extracting it into a helper function.
func createInboundEvent(deposit *solanacontracts.Deposit, tx *solana.Transaction, senderChainID int64, coinType coin.CoinType) *clienttypes.InboundEvent { return &clienttypes.InboundEvent{ SenderChainID: senderChainID, Sender: deposit.Sender, Receiver: "", // receiver will be pulled out from memo later TxOrigin: deposit.Sender, Amount: deposit.Amount, Memo: deposit.Memo, BlockNumber: deposit.Slot, TxHash: tx.Signatures[0].String(), Index: 0, CoinType: coinType, Asset: deposit.Asset, } }Also applies to: 261-272
🧹 Nitpick comments (12)
cmd/zetatool/config/config.go (1)
112-120: Ensure comprehensive handling of all network types inGetConfigThe
GetConfigfunction mapschain.NetworkTypeto specific configurations. If an unsupported or unrecognizedNetworkTypeis provided, the function will returnnil, potentially leading to a nil pointer dereference. Consider adding a default case or error handling to manage unrecognized network types gracefully.Apply this diff to improve the function:
func GetConfig(chain chains.Chain, filename string) (*Config, error) { // Check if cfgFile is empty, if so return default Config and save to file if filename == "" { return map[chains.NetworkType]*Config{ chains.NetworkType_mainnet: MainnetConfig(), chains.NetworkType_testnet: TestnetConfig(), chains.NetworkType_privnet: PrivateNetConfig(), chains.NetworkType_devnet: DevnetConfig(), - }[chain.NetworkType], nil + }[chain.NetworkType], nil + if cfg == nil { + return nil, fmt.Errorf("unsupported network type: %v", chain.NetworkType) + } } // if file is specified, open file and return struct cfg := &Config{} err := cfg.Read(filename) return cfg, err }cmd/zetatool/inbound_ballot/evm.go (3)
151-151: Correct typographical error in error messageThe error message contains a typographical error: "trasnaction" should be "transaction".
Apply this diff to correct the typo:
- return "", fmt.Errorf("irrelevant trasnaction , not sent to any known address txHash: %s", inboundHash) + return "", fmt.Errorf("irrelevant transaction, not sent to any known address txHash: %s", inboundHash)
191-191: Ensure consistent message encoding across vote messagesIn the
zetaTokenVoteV1function, themessageis base64-encoded, whereas in other functions likeerc20VoteV1andgasVoteV1, the message is hex-encoded. For consistency and to avoid potential decoding issues, consider using the same encoding method across all vote messages.
229-229: Align message encoding with other vote message functionsIn the
erc20VoteV1function, themessageis hex-encoded. To maintain consistency across the codebase, consider using base64 encoding as used inzetaTokenVoteV1, or standardize on one encoding method for all messages.cmd/zetatool/inbound_ballot/inbound.go (4)
29-29: Provide detailed error message when parsing chain ID failsIncluding the input value and the underlying error in the error message can aid in debugging and improve user feedback.
Apply this diff to enhance the error message:
inboundChainID, err := strconv.ParseInt(args[1], 10, 64) if err != nil { - return fmt.Errorf("failed to parse chain id") + return fmt.Errorf("failed to parse chain id '%s': %v", args[1], err) }
69-69: Standardize chain name capitalization in error messagesIn the error message, "evm chain" should be capitalized as "EVM chain" to maintain consistency and proper naming conventions.
Apply this diff:
return fmt.Errorf( - "failed to get inbound ballot for evm chain %d, %s", + "failed to get inbound ballot for EVM chain %d, %s", observationChain.ChainId, err.Error(), )
87-87: Standardize chain name capitalization in error messagesIn the error message, "bitcoin chain" should be capitalized as "Bitcoin chain" to adhere to proper naming conventions.
Apply this diff:
return fmt.Errorf( - "failed to get inbound ballot for bitcoin chain %d, %s", + "failed to get inbound ballot for Bitcoin chain %d, %s", observationChain.ChainId, err.Error(), )
105-105: Standardize chain name capitalization in error messagesIn the error message, "solana chain" should be capitalized as "Solana chain" to maintain consistency and proper naming conventions.
Apply this diff:
return fmt.Errorf( - "failed to get inbound ballot for solana chain %d, %s", + "failed to get inbound ballot for Solana chain %d, %s", observationChain.ChainId, err.Error(), )zetaclient/chains/solana/observer/inbound.go (4)
180-182: Enhance error context in wrapper method.Consider wrapping the error with additional context to maintain the error chain.
func (ob *Observer) FilterInboundEvents(txResult *rpc.GetTransactionResult) ([]*clienttypes.InboundEvent, error) { - return FilterSolanaInboundEvents(txResult, ob.Logger(), ob.gatewayID, ob.Chain().ChainId) + events, err := FilterSolanaInboundEvents(txResult, ob.Logger(), ob.gatewayID, ob.Chain().ChainId) + if err != nil { + return nil, fmt.Errorf("failed to filter Solana inbound events: %w", err) + } + return events, nil }
189-192: Consider splitting function into smaller, more focused components.The function has high cyclomatic complexity. Consider extracting the deposit handling logic into separate functions.
Example structure:
func FilterSolanaInboundEvents(txResult *rpc.GetTransactionResult, logger *base.ObserverLogger, gatewayID solana.PublicKey, senderChainID int64) ([]*clienttypes.InboundEvent, error) { tx, err := validateAndUnmarshalTx(txResult) if err != nil { return nil, err } return processInstructions(tx, logger, gatewayID, senderChainID) } func processInstructions(tx *solana.Transaction, logger *base.ObserverLogger, gatewayID solana.PublicKey, senderChainID int64) ([]*clienttypes.InboundEvent, error) { // ... instruction processing logic }
245-247: Consolidate logging patterns using structured logging.The logging patterns are duplicated. Consider creating a helper function for consistent logging.
func logDepositDetection(logger *base.ObserverLogger, depositType string, signature string, instructionIndex int) { logger.Inbound.Info(). Msgf("FilterInboundEvents: %s deposit detected in sig %s instruction %d", depositType, signature, instructionIndex) }Also applies to: 273-275
233-233: Document magic numbers and consider making them constants.The hardcoded
Index: 0appears in multiple places. Consider adding a constant with documentation explaining its purpose.const ( // SolanaEventIndex represents the event index for Solana transactions. // It's hardcoded to 0 as Solana doesn't use smart contract event indexing like EVM chains. SolanaEventIndex = 0 )Also applies to: 261-261
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
changelog.md(1 hunks)cmd/zetaclientd/inbound.go(0 hunks)cmd/zetaclientd/main.go(0 hunks)cmd/zetatool/config/config.go(2 hunks)cmd/zetatool/config/config_test.go(0 hunks)cmd/zetatool/filterdeposit/btc.go(0 hunks)cmd/zetatool/filterdeposit/evm.go(0 hunks)cmd/zetatool/filterdeposit/filterdeposit.go(0 hunks)cmd/zetatool/filterdeposit/filterdeposit_test.go(0 hunks)cmd/zetatool/inbound_ballot/bitcoin.go(1 hunks)cmd/zetatool/inbound_ballot/evm.go(1 hunks)cmd/zetatool/inbound_ballot/inbound.go(1 hunks)cmd/zetatool/inbound_ballot/solana.go(1 hunks)cmd/zetatool/main.go(2 hunks)zetaclient/chains/solana/observer/inbound.go(7 hunks)
💤 Files with no reviewable changes (7)
- cmd/zetatool/filterdeposit/evm.go
- cmd/zetaclientd/main.go
- cmd/zetatool/config/config_test.go
- cmd/zetaclientd/inbound.go
- cmd/zetatool/filterdeposit/filterdeposit_test.go
- cmd/zetatool/filterdeposit/filterdeposit.go
- cmd/zetatool/filterdeposit/btc.go
✅ Files skipped from review due to trivial changes (1)
- changelog.md
🧰 Additional context used
📓 Path-based instructions (7)
cmd/zetatool/main.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetatool/inbound_ballot/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetatool/config/config.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetatool/inbound_ballot/bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetatool/inbound_ballot/solana.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetatool/inbound_ballot/evm.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (2)
cmd/zetatool/inbound_ballot/bitcoin.go (1)
70-120: Refactor shared logic to enhance code reuse and maintainabilityThe function
bitcoinBallotIdentifiercontains logic that is similar to other chain-specific ballot identification functions. To reduce code duplication and improve maintainability, consider abstracting the common logic into shared utility functions or interfaces that can be used across different blockchain implementations.cmd/zetatool/main.go (1)
25-25: Improved error output to standard error streamRedirecting error messages to
os.Stderrusingfmt.Fprintfenhances error handling by ensuring that error messages are appropriately separated from standard output, following best practices.
|
|
||
| func NewFetchInboundBallotCMD() *cobra.Command { | ||
| return &cobra.Command{ | ||
| Use: "get-ballot [inboundHash] [chainID]", |
There was a problem hiding this comment.
Instead of using config, would be simpler to just directly pass the argument here. For EVM and most chain, you just need a RPC? Command would be simpler to use as
get-ballot hash chain_id rpc
rather than setting up a config we can also set a default public rpc used automatically for each chain
There was a problem hiding this comment.
We have a default value for now for each chain; the config is not mandatory.
I think we should just make sure the default values are working, having just two parameters is optimal . Specially if we want to use a HTTP server in the future instead of the CLI
| zetacoreClient rpc.Clients, | ||
| inboundHash string, | ||
| inboundChain chains.Chain, | ||
| zetaChainID int64) (string, error) { |
There was a problem hiding this comment.
I think this comes up a lot for naming
ChainID vs ChainId
We have both in the codebase; I lean more towards ChainID when defining variable names, please comment on this thread if you disagree
There was a problem hiding this comment.
foreignChainID would be my preference when referring to foreign chains.
But it's hard to find the right thing because we have our own internal IDs too. zetaForeignChainID in that case?
| zetacoreClient rpc.Clients, | ||
| inboundHash string, | ||
| inboundChain chains.Chain, | ||
| zetaChainID int64) (string, error) { |
There was a problem hiding this comment.
foreignChainID would be my preference when referring to foreign chains.
But it's hard to find the right thing because we have our own internal IDs too. zetaForeignChainID in that case?
cmd/zetatool/inbound/bitcoin.go
Outdated
| connCfg := &rpcclient.ConnConfig{ | ||
| Host: cfg.BtcHost, | ||
| User: cfg.BtcUser, | ||
| Pass: cfg.BtcPassword, | ||
| HTTPPostMode: true, | ||
| DisableTLS: true, | ||
| Params: params.Name, | ||
| } | ||
| rpcClient, err := rpcclient.New(connCfg, nil) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error creating rpc client: %s", err) | ||
| } |
There was a problem hiding this comment.
Please use the new internal bitcoin rpc client introduced in #3349 if possible
cmd/zetatool/inbound/bitcoin.go
Outdated
| } | ||
| res, err := zetacoreClient.Observer.GetTssAddress(context.Background(), &types.QueryGetTssAddressRequest{}) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get tss address %s", err.Error()) |
There was a problem hiding this comment.
do not squash the error please
kingpinXD
changed the title
Fixed the formating , will open for review again after updating the btc client |
Description
How Has This Been Tested?
Summary by CodeRabbit
Based on the provided summary, here are the release notes:
Refactor
New Features
Chores