From 3d44b5363324baa803a2d9523ce11ded7cce1ed1 Mon Sep 17 00:00:00 2001 From: Michael FIG Date: Wed, 9 Oct 2024 14:38:05 -0600 Subject: [PATCH] feat(vtransfer): extract base address from parameterized address --- .../cosmos/x/vtransfer/ibc_middleware_test.go | 7 +- golang/cosmos/x/vtransfer/keeper/keeper.go | 77 ++++---- golang/cosmos/x/vtransfer/types/baseaddr.go | 156 ++++++++++++++++ .../cosmos/x/vtransfer/types/baseaddr_test.go | 167 ++++++++++++++++++ 4 files changed, 366 insertions(+), 41 deletions(-) create mode 100644 golang/cosmos/x/vtransfer/types/baseaddr.go create mode 100644 golang/cosmos/x/vtransfer/types/baseaddr_test.go diff --git a/golang/cosmos/x/vtransfer/ibc_middleware_test.go b/golang/cosmos/x/vtransfer/ibc_middleware_test.go index ab82e14e09d..23279bf8a63 100644 --- a/golang/cosmos/x/vtransfer/ibc_middleware_test.go +++ b/golang/cosmos/x/vtransfer/ibc_middleware_test.go @@ -331,18 +331,19 @@ func (s *IntegrationTestSuite) TestTransferFromAgdToAgd() { s.Run("TransferFromAgdToAgd", func() { // create a transfer packet's data contents + baseReceiver := s.chainB.SenderAccounts[1].SenderAccount.GetAddress().String() transferData := ibctransfertypes.NewFungibleTokenPacketData( "uosmo", "1000000", s.chainA.SenderAccount.GetAddress().String(), - s.chainB.SenderAccounts[1].SenderAccount.GetAddress().String(), + baseReceiver+"?what=arbitrary-data&why=to-test-bridge-targets", `"This is a JSON memo"`, ) // Register the sender and receiver as bridge targets on their specific // chain. s.RegisterBridgeTarget(s.chainA, transferData.Sender) - s.RegisterBridgeTarget(s.chainB, transferData.Receiver) + s.RegisterBridgeTarget(s.chainB, baseReceiver) s.mintToAddress(s.chainA, s.chainA.SenderAccount.GetAddress(), transferData.Denom, transferData.Amount) @@ -384,7 +385,7 @@ func (s *IntegrationTestSuite) TestTransferFromAgdToAgd() { BlockTime: writeAcknowledgementTime, }, Event: "writeAcknowledgement", - Target: transferData.Receiver, + Target: baseReceiver, Packet: packet, Acknowledgement: ack.Acknowledgement(), }, diff --git a/golang/cosmos/x/vtransfer/keeper/keeper.go b/golang/cosmos/x/vtransfer/keeper/keeper.go index 36701a544e7..0d99840c061 100644 --- a/golang/cosmos/x/vtransfer/keeper/keeper.go +++ b/golang/cosmos/x/vtransfer/keeper/keeper.go @@ -16,7 +16,7 @@ import ( "github.com/Agoric/agoric-sdk/golang/cosmos/vm" "github.com/Agoric/agoric-sdk/golang/cosmos/x/vibc" vibctypes "github.com/Agoric/agoric-sdk/golang/cosmos/x/vibc/types" - transfertypes "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types" + "github.com/Agoric/agoric-sdk/golang/cosmos/x/vtransfer/types" channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types" porttypes "github.com/cosmos/ibc-go/v6/modules/core/05-port/types" host "github.com/cosmos/ibc-go/v6/modules/core/24-host" @@ -106,7 +106,13 @@ func (k Keeper) GetReceiverImpl() vibctypes.ReceiverImpl { // to tell the IBC system that acknowledgment is async (i.e., that WriteAcknowledgement // will be called later, after the VM has dealt with the packet). func (k Keeper) InterceptOnRecvPacket(ctx sdk.Context, ibcModule porttypes.IBCModule, packet channeltypes.Packet, relayer sdk.AccAddress) ibcexported.Acknowledgement { - ack := ibcModule.OnRecvPacket(ctx, packet, relayer) + // Pass every (stripped-receiver) inbound packet to the wrapped IBC module. + var strippedPacket channeltypes.Packet + _, err := types.ExtractBaseAddressFromPacket(k.cdc, packet, types.RoleReceiver, &strippedPacket) + if err != nil { + return channeltypes.NewErrorAcknowledgement(err) + } + ack := ibcModule.OnRecvPacket(ctx, strippedPacket, relayer) if ack == nil { // Already declared to be an async ack. @@ -136,17 +142,21 @@ func (k Keeper) InterceptOnAcknowledgementPacket( acknowledgement []byte, relayer sdk.AccAddress, ) error { - // Pass every acknowledgement to the wrapped IBC module. - modErr := ibcModule.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer) + // Pass every (stripped-sender) acknowledgement to the wrapped IBC module. + var strippedPacket channeltypes.Packet + baseSender, err := types.ExtractBaseAddressFromPacket(k.cdc, packet, types.RoleSender, &strippedPacket) + if err != nil { + return err + } + modErr := ibcModule.OnAcknowledgementPacket(ctx, strippedPacket, acknowledgement, relayer) - // If the sender is not a targeted account, we're done. - sender, _, err := k.parseTransfer(ctx, packet) - if err != nil || sender == "" { + // If the sender is not a watched account, we're done. + if !k.targetIsWatched(ctx, baseSender) { return modErr } - // Trigger VM, regardless of errors in the ibcModule. - vmErr := k.vibcKeeper.TriggerOnAcknowledgementPacket(ctx, sender, packet, acknowledgement, relayer) + // Trigger VM with the original packet, regardless of errors in the ibcModule. + vmErr := k.vibcKeeper.TriggerOnAcknowledgementPacket(ctx, baseSender, packet, acknowledgement, relayer) // Any error from the VM is trumped by one from the wrapped IBC module. if modErr != nil { @@ -163,17 +173,21 @@ func (k Keeper) InterceptOnTimeoutPacket( packet channeltypes.Packet, relayer sdk.AccAddress, ) error { - // Pass every timeout to the wrapped IBC module. - modErr := ibcModule.OnTimeoutPacket(ctx, packet, relayer) + // Pass every (stripped-sender) timeout to the wrapped IBC module. + var strippedPacket channeltypes.Packet + baseSender, err := types.ExtractBaseAddressFromPacket(k.cdc, packet, types.RoleSender, &strippedPacket) + if err != nil { + return err + } + modErr := ibcModule.OnTimeoutPacket(ctx, strippedPacket, relayer) - // If the sender is not a targeted account, we're done. - sender, _, err := k.parseTransfer(ctx, packet) - if err != nil || sender == "" { + // If the sender is not a watched account, we're done. + if !k.targetIsWatched(ctx, baseSender) { return modErr } - // Trigger VM, regardless of errors in the app. - vmErr := k.vibcKeeper.TriggerOnTimeoutPacket(ctx, sender, packet, relayer) + // Trigger VM with the original packet, regardless of errors in the app. + vmErr := k.vibcKeeper.TriggerOnTimeoutPacket(ctx, baseSender, packet, relayer) // Any error from the VM is trumped by one from the wrapped IBC module. if modErr != nil { @@ -185,14 +199,15 @@ func (k Keeper) InterceptOnTimeoutPacket( // InterceptWriteAcknowledgement checks to see if the packet's receiver is a // targeted account, and if so, delegates to the VM. func (k Keeper) InterceptWriteAcknowledgement(ctx sdk.Context, chanCap *capabilitytypes.Capability, packet ibcexported.PacketI, ack ibcexported.Acknowledgement) error { - _, receiver, err := k.parseTransfer(ctx, packet) - if err != nil || receiver == "" { - // We can't parse, but that means just to ack directly. + // Get the base baseReceiver from the packet, without computing a stripped packet. + baseReceiver, err := types.ExtractBaseAddressFromPacket(k.cdc, packet, types.RoleReceiver, nil) + if err != nil || !k.targetIsWatched(ctx, baseReceiver) { + // We can't parse, or not watching, but that means just to ack directly. return k.WriteAcknowledgement(ctx, chanCap, packet, ack) } - // Trigger VM - if err = k.vibcKeeper.TriggerWriteAcknowledgement(ctx, receiver, packet, ack); err != nil { + // Trigger VM with the original packet. + if err = k.vibcKeeper.TriggerWriteAcknowledgement(ctx, baseReceiver, packet, ack); err != nil { errAck := channeltypes.NewErrorAcknowledgement(err) return k.WriteAcknowledgement(ctx, chanCap, packet, errAck) } @@ -200,27 +215,13 @@ func (k Keeper) InterceptWriteAcknowledgement(ctx sdk.Context, chanCap *capabili return nil } -// parseTransfer checks if a packet's sender and/or receiver are targeted accounts. -func (k Keeper) parseTransfer(ctx sdk.Context, packet ibcexported.PacketI) (string, string, error) { - var transferData transfertypes.FungibleTokenPacketData - err := k.cdc.UnmarshalJSON(packet.GetData(), &transferData) - if err != nil { - return "", "", err - } - - var sender string - var receiver string +// targetIsWatched checks if a target address has been watched by the VM. +func (k Keeper) targetIsWatched(ctx sdk.Context, target string) bool { prefixStore := prefix.NewStore( ctx.KVStore(k.key), []byte(watchedAddressStoreKeyPrefix), ) - if prefixStore.Has([]byte(transferData.Sender)) { - sender = transferData.Sender - } - if prefixStore.Has([]byte(transferData.Receiver)) { - receiver = transferData.Receiver - } - return sender, receiver, nil + return prefixStore.Has([]byte(target)) } // GetWatchedAdresses returns the watched addresses from the keeper as a slice diff --git a/golang/cosmos/x/vtransfer/types/baseaddr.go b/golang/cosmos/x/vtransfer/types/baseaddr.go new file mode 100644 index 00000000000..7ad65541746 --- /dev/null +++ b/golang/cosmos/x/vtransfer/types/baseaddr.go @@ -0,0 +1,156 @@ +package types + +import ( + "fmt" + "net/url" + "strings" + + "github.com/cosmos/cosmos-sdk/codec" + + transfertypes "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types" + clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" + channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types" + ibcexported "github.com/cosmos/ibc-go/v6/modules/core/exported" +) + +type AddressRole string + +const ( + RoleSender AddressRole = "Sender" + RoleReceiver AddressRole = "Receiver" +) + +func trimSlashPrefix(s string) string { + return strings.TrimPrefix(s, "/") +} + +// ExtractBaseAddress extracts the base address from a parameterized address. +// It removes all subpath and query components from addr. +func ExtractBaseAddress(addr string) (string, error) { + parsed, err := url.Parse(addr) + if err != nil { + return "", err + } + + // Specify the fields and values we expect. Unspecified fields will only + // match if they are zero values in order to be robust against extensions to + // the url.URL struct. + // + // Remove leading slashes from the path fields so that only parsed relative + // paths match the expected test. + expected := url.URL{ + Path: trimSlashPrefix(parsed.Path), + RawPath: trimSlashPrefix(parsed.RawPath), + RawQuery: parsed.RawQuery, + Fragment: parsed.Fragment, + RawFragment: parsed.RawFragment, + + // Skip over parsing control flags. + ForceQuery: parsed.ForceQuery, + OmitHost: parsed.OmitHost, + } + + if *parsed != expected { + return "", fmt.Errorf("address must be relative path with optional query and fragment, got %s", addr) + } + + baseAddr, _, _ := strings.Cut(expected.Path, "/") + if baseAddr == "" { + return "", fmt.Errorf("base address cannot be empty") + } + + return baseAddr, nil +} + +// extractBaseTransferData returns the base address from the transferData.Sender +// (if RoleSender) or transferData.Receiver (if RoleReceiver). Errors in +// determining the base address are ignored... we then assume the base address +// is exactly the original address. If newTransferData is not nil, it will be +// populated with a new FungibleTokenPacketData consisting of the role replaced +// with its base address. +func extractBaseTransferData(transferData transfertypes.FungibleTokenPacketData, role AddressRole, newTransferData *transfertypes.FungibleTokenPacketData) (string, error) { + var target string + sender := transferData.Sender + receiver := transferData.Receiver + + switch role { + case RoleSender: + baseSender, err := ExtractBaseAddress(sender) + if err == nil { + sender = baseSender + } + target = sender + + case RoleReceiver: + baseReceiver, err := ExtractBaseAddress(receiver) + if err == nil { + receiver = baseReceiver + } + target = receiver + + default: + err := fmt.Errorf("invalid address role: %s", role) + return target, err + } + + if newTransferData == nil { + return target, nil + } + + // Create the new transfer data. + *newTransferData = transfertypes.NewFungibleTokenPacketData( + transferData.Denom, + transferData.Amount, + sender, receiver, + transferData.Memo, + ) + + return target, nil +} + +// ExtractBaseAddressFromPacket returns the base address from a transfer +// packet's data, either Sender (if role is RoleSender) or Receiver (if role is +// RoleReceiver). +// Errors in determining the base address are ignored... we then assume the base +// address is exactly the original address. +// If newPacket is not nil, it is populated with a new transfer packet whose +// corresponding Sender or Receiver is replaced with the extracted base address. +func ExtractBaseAddressFromPacket(cdc codec.Codec, packet ibcexported.PacketI, role AddressRole, newPacket *channeltypes.Packet) (string, error) { + transferData := transfertypes.FungibleTokenPacketData{} + if err := cdc.UnmarshalJSON(packet.GetData(), &transferData); err != nil { + return "", err + } + + var newTransferData *transfertypes.FungibleTokenPacketData + if newPacket != nil { + // Capture the transfer data for the new packet. + newTransferData = &transfertypes.FungibleTokenPacketData{} + } + target, err := extractBaseTransferData(transferData, role, newTransferData) + if err != nil { + return target, err + } + + if newPacket == nil { + return target, nil + } + + // Create a new packet with the new transfer packet data. + // Re-serialize the packet data with the base addresses. + newData, err := cdc.MarshalJSON(newTransferData) + if err != nil { + return target, err + } + + // Create the new packet. + th := packet.GetTimeoutHeight() + *newPacket = channeltypes.NewPacket( + newData, packet.GetSequence(), + packet.GetSourcePort(), packet.GetSourceChannel(), + packet.GetDestPort(), packet.GetDestChannel(), + clienttypes.NewHeight(th.GetRevisionNumber(), th.GetRevisionHeight()), + packet.GetTimeoutTimestamp(), + ) + + return target, nil +} diff --git a/golang/cosmos/x/vtransfer/types/baseaddr_test.go b/golang/cosmos/x/vtransfer/types/baseaddr_test.go new file mode 100644 index 00000000000..eb83f07ddb6 --- /dev/null +++ b/golang/cosmos/x/vtransfer/types/baseaddr_test.go @@ -0,0 +1,167 @@ +package types_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + codec "github.com/cosmos/cosmos-sdk/codec" + cdctypes "github.com/cosmos/cosmos-sdk/codec/types" + + transfertypes "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types" + clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" + channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types" + + "github.com/Agoric/agoric-sdk/golang/cosmos/x/vtransfer/types" +) + +func TestExtractBaseAddress(t *testing.T) { + bases := []struct { + name string + addr string + }{ + {"agoric address", "agoric1abcdefghiteaneas"}, + {"cosmos address", "cosmos1abcdeffiharceuht"}, + {"hex address", "0xabcdef198189818c93839ibia"}, + } + + prefixes := []struct { + prefix string + baseIsWrong bool + isErr bool + }{ + {"", false, false}, + {"/", false, true}, + {"orch:/", false, true}, + {"unexpected", true, false}, + {"norch:/", false, true}, + {"orch:", false, true}, + {"norch:", false, true}, + {"\x01", false, true}, + } + + suffixes := []struct { + suffix string + baseIsWrong bool + isErr bool + }{ + {"", false, false}, + {"/", false, false}, + {"/sub/account", false, false}, + {"?query=something&k=v&k2=v2", false, false}, + {"?query=something&k=v&k2=v2#fragment", false, false}, + {"unexpected", true, false}, + {"\x01", false, true}, + } + + for _, b := range bases { + b := b + for _, p := range prefixes { + p := p + for _, s := range suffixes { + s := s + t.Run(b.name+" "+p.prefix+" "+s.suffix, func(t *testing.T) { + addr := p.prefix + b.addr + s.suffix + addr, err := types.ExtractBaseAddress(addr) + if p.isErr || s.isErr { + require.Error(t, err) + } else { + require.NoError(t, err) + if p.baseIsWrong || s.baseIsWrong { + require.NotEqual(t, b.addr, addr) + } else { + require.Equal(t, b.addr, addr) + } + } + }) + } + } + } +} + +func TestExtractBaseAddressFromPacket(t *testing.T) { + ir := cdctypes.NewInterfaceRegistry() + cdc := codec.NewProtoCodec(ir) + transfertypes.RegisterInterfaces(ir) + channeltypes.RegisterInterfaces(ir) + clienttypes.RegisterInterfaces(ir) + + cases := []struct { + name string + addrs map[types.AddressRole]struct{ addr, baseAddr string } + }{ + {"sender has params", + map[types.AddressRole]struct{ addr, baseAddr string }{ + types.RoleSender: {"cosmos1abcdeffiharceuht?foo=bar&baz=bot#fragment", "cosmos1abcdeffiharceuht"}, + types.RoleReceiver: {"agoric1abcdefghiteaneas", "agoric1abcdefghiteaneas"}, + }, + }, + {"receiver has params", + map[types.AddressRole]struct{ addr, baseAddr string }{ + types.RoleSender: {"cosmos1abcdeffiharceuht", "cosmos1abcdeffiharceuht"}, + types.RoleReceiver: {"agoric1abcdefghiteaneas?bingo=again", "agoric1abcdefghiteaneas"}, + }, + }, + {"both are base", + map[types.AddressRole]struct{ addr, baseAddr string }{ + types.RoleSender: {"cosmos1abcdeffiharceuht", "cosmos1abcdeffiharceuht"}, + types.RoleReceiver: {"agoric1abcdefghiteaneas", "agoric1abcdefghiteaneas"}, + }, + }, + {"both have params", + map[types.AddressRole]struct{ addr, baseAddr string }{ + types.RoleSender: {"agoric1abcdefghiteaneas?bingo=again", "agoric1abcdefghiteaneas"}, + types.RoleReceiver: {"cosmos1abcdeffiharceuht?foo=bar&baz=bot#fragment", "cosmos1abcdeffiharceuht"}, + }, + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + ftPacketData := transfertypes.NewFungibleTokenPacketData("denom", "100", tc.addrs[types.RoleSender].addr, tc.addrs[types.RoleReceiver].addr, "my-favourite-memo") + packetBz, err := cdc.MarshalJSON(&ftPacketData) + require.NoError(t, err) + packet := channeltypes.NewPacket(packetBz, 1234, "my-port", "my-channel", "their-port", "their-channel", clienttypes.NewHeight(133, 445), 10999) + + for role, addrs := range tc.addrs { + addrs := addrs + role := role + + t.Run(string(role), func(t *testing.T) { + baseAddr, err := types.ExtractBaseAddress(addrs.addr) + require.NoError(t, err) + require.Equal(t, addrs.baseAddr, baseAddr) + + packetBaseAddr, err := types.ExtractBaseAddressFromPacket(cdc, packet, role, nil) + require.NoError(t, err) + require.Equal(t, addrs.baseAddr, packetBaseAddr) + + var newPacket channeltypes.Packet + packetBaseAddr2, err := types.ExtractBaseAddressFromPacket(cdc, packet, role, &newPacket) + require.NoError(t, err) + require.Equal(t, addrs.baseAddr, packetBaseAddr2) + + var basePacketData transfertypes.FungibleTokenPacketData + err = cdc.UnmarshalJSON(newPacket.GetData(), &basePacketData) + require.NoError(t, err) + + // Check that the only difference between the packet data is the baseAddr. + packetData := basePacketData + switch role { + case types.RoleSender: + require.Equal(t, addrs.baseAddr, basePacketData.Sender) + packetData.Sender = addrs.addr + case types.RoleReceiver: + require.Equal(t, addrs.baseAddr, basePacketData.Receiver) + packetData.Receiver = addrs.addr + default: + t.Fatal("unexpected role", role) + } + + require.Equal(t, ftPacketData, packetData) + }) + } + }) + } +}