feat: Verify transfer addresses in Observer#7943
Conversation
Kormick
added
V:state/compatible/no_backport
WalkthroughThis update focuses on enhancing the functionality and reliability of the Bitcoin and Osmosis observers within a blockchain bridge system. For Bitcoin, it involves refining transaction relevance checks and memo retrieval, including address validation. For Osmosis, enhancements include the introduction of operating modes, improved error handling, and outbound address verification. Both sets of changes aim to improve the system's robustness and its ability to handle transactions more effectively. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| func verifyOutboundDestAddress(mode Mode, chainId observer.ChainId, addr string) error { | ||
| switch chainId { | ||
| case observer.ChainIdBitcoin: | ||
| switch mode { | ||
| case ModeMainnet: | ||
| _, err := btcutil.DecodeAddress(addr, &btcdchaincfg.MainNetParams) | ||
| return err | ||
| case ModeTestnet: | ||
| _, err := btcutil.DecodeAddress(addr, &btcdchaincfg.TestNet3Params) | ||
| return err | ||
| } | ||
| } | ||
| return fmt.Errorf("Unsupported outbound destination chain: %s", chainId) | ||
| } |
There was a problem hiding this comment.
Add a default case in the switch statement for chainId to handle unsupported chains more gracefully in the verifyOutboundDestAddress function.
switch chainId {
case observer.ChainIdBitcoin:
// existing code
+default:
+ return fmt.Errorf("verifyOutboundDestAddress: unsupported chain ID %s", chainId)
}This change improves error handling by providing a clear error message for unsupported chains.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func verifyOutboundDestAddress(mode Mode, chainId observer.ChainId, addr string) error { | |
| switch chainId { | |
| case observer.ChainIdBitcoin: | |
| switch mode { | |
| case ModeMainnet: | |
| _, err := btcutil.DecodeAddress(addr, &btcdchaincfg.MainNetParams) | |
| return err | |
| case ModeTestnet: | |
| _, err := btcutil.DecodeAddress(addr, &btcdchaincfg.TestNet3Params) | |
| return err | |
| } | |
| } | |
| return fmt.Errorf("Unsupported outbound destination chain: %s", chainId) | |
| } | |
| func verifyOutboundDestAddress(mode Mode, chainId observer.ChainId, addr string) error { | |
| switch chainId { | |
| case observer.ChainIdBitcoin: | |
| switch mode { | |
| case ModeMainnet: | |
| _, err := btcutil.DecodeAddress(addr, &btcdchaincfg.MainNetParams) | |
| return err | |
| case ModeTestnet: | |
| _, err := btcutil.DecodeAddress(addr, &btcdchaincfg.TestNet3Params) | |
| return err | |
| } | |
| default: | |
| return fmt.Errorf("verifyOutboundDestAddress: unsupported chain ID %s", chainId) | |
| } | |
| return fmt.Errorf("Unsupported outbound destination chain: %s", chainId) | |
| } |
| memo, contains := getMemo(tx) | ||
| if !contains { | ||
| return observer.Transfer{}, false, fmt.Errorf("failed to get Tx memo") | ||
| return observer.Transfer{}, false, fmt.Errorf("malformed Tx memo") |
There was a problem hiding this comment.
Ensure proper logging for the error case in the getMemo function when a transaction memo is malformed.
Adding a log statement here can aid in debugging and operational monitoring.
if !contains {
+ b.logger.Error("Malformed transaction memo", "txid", tx.Txid)
return observer.Transfer{}, false, fmt.Errorf("malformed Tx memo")
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return observer.Transfer{}, false, fmt.Errorf("malformed Tx memo") | |
| b.logger.Error("Malformed transaction memo", "txid", tx.Txid) | |
| return observer.Transfer{}, false, fmt.Errorf("malformed Tx memo") |
| // We expect Observer to observe 1 block with 8 Txs: | ||
| // - tx to our vault but without memo | ||
| // - tx to our vault with memo with invalid address | ||
| // - tx to our vault but with zero tokens | ||
| // - tx to our vault with invalid script type for output Vout | ||
| // - tx to our vault with invalid script type for memo Vout | ||
| // - tx to our vault with invalid tokens amount | ||
| // - unrelated tx | ||
| // + valid tx to our vault | ||
| // So, we should receive only 1 Transfer |
* feat: Verify transfer addresses * chore: typo (cherry picked from commit 1b3449b)
2 participants
Epic: #7643
What is the purpose of the change
Testing and Verifying
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)Summary by CodeRabbit
New Features
Bug Fixes
Refactor