Skip to content
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

feat(vtransfer): extract base address from parameterized address #10250

Merged
merged 1 commit into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions golang/cosmos/x/vtransfer/ibc_middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -384,7 +385,7 @@ func (s *IntegrationTestSuite) TestTransferFromAgdToAgd() {
BlockTime: writeAcknowledgementTime,
},
Event: "writeAcknowledgement",
Target: transferData.Receiver,
Target: baseReceiver,
Packet: packet,
Acknowledgement: ack.Acknowledgement(),
},
Expand Down
77 changes: 39 additions & 38 deletions golang/cosmos/x/vtransfer/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"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"
Expand Down Expand Up @@ -106,7 +106,13 @@
// 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.
Expand All @@ -117,7 +123,7 @@
capName := host.ChannelCapabilityPath(portID, channelID)
chanCap, ok := k.vibcKeeper.GetCapability(ctx, capName)
if !ok {
err := sdkerrors.Wrapf(channeltypes.ErrChannelCapabilityNotFound, "could not retrieve channel capability at: %s", capName)

Check failure on line 126 in golang/cosmos/x/vtransfer/keeper/keeper.go

View workflow job for this annotation

GitHub Actions / golangci-lint (no-failure)

SA1019: sdkerrors.Wrapf is deprecated: functionality of this package has been moved to it's own module: (staticcheck)
return channeltypes.NewErrorAcknowledgement(err)
}
// Give the VM a chance to write (or override) the ack.
Expand All @@ -136,17 +142,21 @@
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 {
Expand All @@ -163,17 +173,21 @@
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 {
Expand All @@ -185,42 +199,29 @@
// 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)
}

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 {
Comment on lines -203 to +219
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is such a good change. 👍

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
Expand Down Expand Up @@ -275,7 +276,7 @@
case "BRIDGE_TARGET_UNREGISTER":
prefixStore.Delete([]byte(msg.Target))
default:
return "", sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unknown action type: %s", msg.Type)

Check failure on line 279 in golang/cosmos/x/vtransfer/keeper/keeper.go

View workflow job for this annotation

GitHub Actions / golangci-lint (no-failure)

SA1019: sdkerrors.Wrapf is deprecated: functionality of this package has been moved to it's own module: (staticcheck)
}
return "true", nil
}
156 changes: 156 additions & 0 deletions golang/cosmos/x/vtransfer/types/baseaddr.go
Original file line number Diff line number Diff line change
@@ -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,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url.URL has more fields than being populated here. We're relying on the other fields like parsed.OmitHost or parsed.ForceQuery to be false, which is the default value for the comparison at line 31 to be comparing what we truly care about.

IMO doing

if ((parsed.Opaque == trimSlashPrefix(parsed.Opaque) && ...)

is still very readable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm relying on all of the non-allowlisted fields to be empty, otherwise to fail. That was the most concise and robust way of ensuring the parsing worked correctly. I had originally done an if-ladder and found it to be much less readable than the declaration of expected and the single comparison of parsed to expected.

However, I'll wait for some other opinions on this, and keep it or change it based on the group's agreement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach and in my inherited Go practice, relying on the zero values for a struct is idiomatic. This is robust against evolution of the struct in a way that an if-ladder would not be, since we want the presence of non-zero values in future fields to result in errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @kriskowal. I also rely on Go zero values, I vote for this approach as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LuqiPan were you pointing out that OmitHost and ForceQuery shouldn't have to be the zero-value, since they are parsing options? I just now carved out an exception for both of these two fields.


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
}
Loading
Loading