-
Notifications
You must be signed in to change notification settings - Fork 693
feat: Verify transfer addresses in Observer #7943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import ( | |
| "github.com/cometbft/cometbft/libs/log" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| _ "github.com/osmosis-labs/osmosis/v24/app/params" // init Osmosis address prefixes | ||
| "github.com/osmosis-labs/osmosis/v24/x/bridge/observer" | ||
| "github.com/osmosis-labs/osmosis/v24/x/bridge/observer/bitcoin" | ||
| bridgetypes "github.com/osmosis-labs/osmosis/v24/x/bridge/types" | ||
|
|
@@ -104,9 +105,16 @@ func TestListenOutboundTransfer(t *testing.T) { | |
| err = b.Start(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| // We expect Observer to observe 1 block with 2 Txs | ||
| // Only 1 Tx is sent to our vault address, | ||
| // so we should receive only 1 TxIn | ||
| // 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 | ||
|
Comment on lines
+108
to
+117
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice |
||
| txs := b.ListenOutboundTransfer() | ||
| var out observer.Transfer | ||
| require.Eventually(t, func() bool { | ||
|
|
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,9 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| errorsmod "cosmossdk.io/errors" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "cosmossdk.io/math" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/btcsuite/btcd/btcutil" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| btcdchaincfg "github.com/btcsuite/btcd/chaincfg" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| abcitypes "github.com/cometbft/cometbft/abci/types" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/cometbft/cometbft/libs/log" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rpchttp "github.com/cometbft/cometbft/rpc/client/http" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| coretypes "github.com/cometbft/cometbft/rpc/core/types" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -19,6 +22,13 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bridgetypes "github.com/osmosis-labs/osmosis/v24/x/bridge/types" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type Mode = string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ModeMainnet Mode = "mainnet" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ModeTestnet Mode = "testnet" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ModuleName = "osmosis-chain" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| OsmoGasLimit = uint64(200000) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -28,6 +38,7 @@ var ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type ChainClient struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger log.Logger | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mode Mode | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| osmoClient *Client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cometRpc *rpchttp.HTTP | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stopChan chan struct{} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -40,13 +51,15 @@ type ChainClient struct { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // NewChainClient returns new instance of `Osmosis` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func NewChainClient( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger log.Logger, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mode Mode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| osmoClient *Client, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cometRpc *rpchttp.HTTP, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| txConfig cosmosclient.TxConfig, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signerAddr string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) *ChainClient { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return &ChainClient{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger: logger.With("module", ModuleName), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mode: mode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| osmoClient: osmoClient, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cometRpc: cometRpc, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stopChan: make(chan struct{}), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -114,14 +127,17 @@ func (c *ChainClient) SignalInboundTransfer(ctx context.Context, in observer.Tra | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return errorsmod.Wrapf(err, "Failed to sign tx for inbound transfer %s", in.Id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _, err = c.osmoClient.BroadcastTx(ctx, bytes) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resp, err := c.osmoClient.BroadcastTx(ctx, bytes) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return errorsmod.Wrapf( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| err, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Failed to broadcast tx to Osmosis for inbound transfer %s", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| in.Id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if resp.Code != abcitypes.CodeTypeOK { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return fmt.Errorf("Tx for inbound transfer %s failed inside Osmosis: %s", in.Id, resp.RawLog) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -170,6 +186,7 @@ func (c *ChainClient) processNewBlockTxs(ctx context.Context, height uint64, txs | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if res.IsErr() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -180,6 +197,21 @@ func (c *ChainClient) processNewBlockTxs(ctx context.Context, height uint64, txs | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| err = verifyOutboundDestAddress( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| c.mode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| observer.ChainId(outbound.AssetId.SourceChain), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| outbound.DestAddr, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| c.logger.Error(fmt.Sprintf( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Invalid outbound destination address in Tx %s, block %d: %s", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| txHash, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| height, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| err.Error(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| out := outboundTransferFromMsg( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| height, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| txHash, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -218,6 +250,21 @@ func outboundTransferFromMsg( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+253
to
+266
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a default case in the switch statement for 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Height returns current height of the chain | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func (c *ChainClient) Height(context.Context) (uint64, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return c.lastObservedHeight.Load(), nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| { | ||
| "code": 32, | ||
| "data": null, | ||
| "log": "account sequence mismatch, expected 10, got 9: incorrect account sequence", | ||
| "info": "", | ||
| "gas_wanted": "200000", | ||
| "gas_used": "39457", | ||
| "events": [], | ||
| "codespace": "sdk", | ||
| "sender": "", | ||
| "priority": "0", | ||
| "mempoolError": "" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper logging for the error case in the
getMemofunction 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