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

michaelfig · 2024-10-09T20:49:45Z

Description

This PR allows ICS-20 fungible token transfer packets to contain inbound receiver and outbound sender addresses formatted as either:

a standard bech32 address ("agoric1bech32addr"), or
a base account with optional subpaths and query parameters ("agoric1bech32addr/opt/account?k=v&k2=v2")

In these cases, the "base address" is "agoric1bech32addr", and tokens are transferred to that base address. The original address can be parsed by a listening contract into "address parameters" to use at its discretion.

Security Considerations

Should be no impact. If a transfer listener does not explicitly look for the data in an inbound packet, it will behave as if there are no address parameters.

Careful review of the code is critical, though, since it touches the powerful x/vtransfer module.

Scaling Considerations

Increases the scalability of contracts that would ordinarily have to maintain a pool of multiple Local Chain Account (LCA) addresses to have a unique ID to map to some parameters. Now, contracts can create just one LCA and have the sender supply the data as part of a parameterized address.

The ICS-20 transfer addresses are strings limited to at most 2048 bytes, and potentially even smaller if explicitly configured by a relayer operator. Thus, the total length of the encoded base address and all the address parameters cannot exceed that address string limit, or the packet will not be forwarded.

Documentation Considerations

Should be documented as a feature of vtransfer.

Testing Considerations

Unit tests with address parameter data tests have been implemented, testing both the address parser code and the IBC end-to-end packet relay process through the x/vtransfer module.

Upgrade Considerations

This is a chain-level, backwards-compatible change, so no upgrade aspects must be addressed. However, Agoric contracts will probably need to import an URL-parsing module to parse URL escaping and query parameters syntax (since the globalThis.URL constructor is not currently available in the SwingSet vat environment).

cloudflare-workers-and-pages · 2024-10-10T00:09:48Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`3d44b53`
Status:	✅ Deploy successful!
Preview URL:	https://0ccc3111.agoric-sdk.pages.dev
Branch Preview URL:	https://mfig-10249-vtransfer-virtual.agoric-sdk.pages.dev

View logs

LuqiPan

I haven't reviewed the code in keeper.go, nor am I an expert in reviewing that. But I did a quick review of the address parsing logic

LuqiPan · 2024-10-31T18:09:45Z

golang/cosmos/x/vtransfer/keeper/keeper.go

+// Extract the base address from the packet sender (if senderIsLocal) or
+// receiver (if !senderIsLocal), since the local ibcModule doesn't understand
+// address parameters.
+func (k Keeper) packetWithOnlyBaseAddresses(packet channeltypes.Packet, senderIsLocal bool) channeltypes.Packet {


question

Could we add some unit tests for this function? If nothing else, just to use unit tests to document the behavior of this function

Definitely! I'll work on those.

LuqiPan · 2024-10-31T18:22:06Z

golang/cosmos/x/vtransfer/types/baseaddr.go

+		RawQuery:    parsed.RawQuery,
+		Fragment:    parsed.Fragment,
+		RawFragment: parsed.RawFragment,
+	}


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.

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.

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.

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

@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.

JeancarloBarrios · 2024-11-04T18:52:14Z

golang/cosmos/x/vtransfer/keeper/keeper.go

+	// Replace the packet's data with the new data.
+	packet.Data = data
+	return packet
+}


Is it expected behavior that when there is an error, we send an empty JSON on Data?

Good catch! No, not expected. I'll fix this while working on its unit tests.

gibson042

The general approach seems good, but I think it could use some tweaks.

golang/cosmos/x/vtransfer/keeper/keeper.go

golang/cosmos/x/vtransfer/types/baseaddr.go

gibson042

The refactoring really works. A few style suggestions, but otherwise LGTM.

gibson042 · 2024-11-07T16:38:45Z

golang/cosmos/x/vtransfer/types/baseaddr.go

+// 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) {


Exporting this function fosters unnecessary confusion w.r.t. ExtractBaseTransferPacket IMO.

Suggested change

// 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) {

// 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) {

gibson042 · 2024-11-07T16:50:24Z

golang/cosmos/x/vtransfer/types/baseaddr.go

+// ExtractBaseTransferPacket returns the base address from the transfer packet's
+// transferData.Sender (if RoleSender) or transferData.Receiver (if
+// RoleReceiver). If newPacket is not nil, it is populated with a new transfer
+// packet whose corresponding Sender/Receiver is replaced with its base address.
+func ExtractBaseTransferPacket(cdc codec.Codec, packet ibcexported.PacketI, role AddressRole, newPacket *channeltypes.Packet) (string, error) {


Suggested rewording and renaming:

Suggested change

// ExtractBaseTransferPacket returns the base address from the transfer packet's

// transferData.Sender (if RoleSender) or transferData.Receiver (if

// RoleReceiver). If newPacket is not nil, it is populated with a new transfer

// packet whose corresponding Sender/Receiver is replaced with its base address.

func ExtractBaseTransferPacket(cdc codec.Codec, packet ibcexported.PacketI, role AddressRole, newPacket *channeltypes.Packet) (string, error) {

// 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) {

gibson042 · 2024-11-07T16:59:30Z

golang/cosmos/x/vtransfer/keeper/keeper.go

-// 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 {


This is such a good change. 👍

closes: #10614 refs: #10249 #10250 ## Description Pack a bech32 "base address" (like "agoric1...") plus HTTP query string "hook parameter" bytes into a bech32 "address hook". These address hooks are bech32-encoded (i.e., all of their data is encoded and part of the bech32 checksum), but they may be up to 1024 characters in length (instead of the default 90-character length limit). ### Example ```js import { encodeAddressHook, decodeAddressHook, } from '@agoric/cosmic-proto/address-hooks.js'; const baseAddress = 'agoric1qqp0e5ys'; const query = { key: 'value', foo: ['bar', 'baz'] }; const addressHook = encodeAddressHook(baseAddress, query); // 'agoric10rchqqplvehk70tzv9ezven0du7kyct6ye4k27faweskcat9qqqstnf2eq' addressHook.startsWith('agoric10rch'); // true const decoded = decodeAddressHook(addressHook); // { // baseAddress: 'agoric1qqp0e5ys', // query: [Object: null prototype] { foo: [ 'bar', 'baz' ], key: 'value' } // } ``` ### Encoding Specifically, an address hook looks like "agoric10rch...", and its binary payload consists of: | offset | 0 | 3 | 3+len(baseAddress) | len(payload)-2 | | ------ | ----- | ----------- | ------------------ | ---------------- | | data | magic | baseAddress | hookData | len(baseAddress) | `magic` is a 3-byte prefix that identifies a hooked address and its version nibble, whose value is 4 bits (between 0 and 0xf (15)). Currently, the only supported version is 0. ```js 0x78, 0xf1, (0x70 | ADDRESS_HOOK_VERSION), ``` This magic prefix encodes as `0rch`, regardless of the version or HRP (e.g. `'agoric10rch<rest of payload as bech32><bech32 checksum>'`). ### Security Considerations Improves robustness of address hook extraction with magic bytes and version nibble, as well as length validation. ### Scaling Considerations n/a ### Documentation Considerations Needs to be documented as part of Orch Core Address Hooks. ### Testing Considerations Should be tested as part of a regular contract. Already verified that encoding/decoding unit tests can pass in a JS compartment without any special powers. ### Upgrade Considerations Layered to facilitate the construction of arbitrary hookData, not just URL query strings. Should be extensible if needed.

michaelfig self-assigned this Oct 9, 2024

michaelfig added the agd Agoric (Golang) Daemon label Oct 9, 2024

michaelfig marked this pull request as ready for review October 10, 2024 00:07

michaelfig requested a review from a team as a code owner October 10, 2024 00:07

michaelfig requested a review from AgoricTriage October 10, 2024 00:07

michaelfig force-pushed the mfig-10249-vtransfer-virtual-targets branch from 98f0cc8 to 792b993 Compare October 10, 2024 00:09

michaelfig force-pushed the mfig-10249-vtransfer-virtual-targets branch 2 times, most recently from 80cdb7b to 63041aa Compare October 10, 2024 19:12

michaelfig requested a review from gibson042 October 17, 2024 18:18

michaelfig force-pushed the mfig-10249-vtransfer-virtual-targets branch 2 times, most recently from de6e865 to 217d5f3 Compare October 17, 2024 19:57

michaelfig changed the title ~~feat(vtransfer): implement +-separated virtual targets~~ feat(vtransfer): extract target account address from parameterized address Oct 25, 2024

michaelfig changed the title ~~feat(vtransfer): extract target account address from parameterized address~~ feat(vtransfer): extract target account address from parameters Oct 25, 2024

michaelfig force-pushed the mfig-10249-vtransfer-virtual-targets branch 8 times, most recently from 2b3b4a3 to 62d0488 Compare October 28, 2024 15:39

michaelfig changed the title ~~feat(vtransfer): extract target account address from parameters~~ feat(vtransfer): extract base address from parameterized address Oct 28, 2024

michaelfig added the force:integration Force integration tests to run on PR label Oct 28, 2024

michaelfig force-pushed the mfig-10249-vtransfer-virtual-targets branch from 62d0488 to 97b28f5 Compare October 28, 2024 18:37

LuqiPan reviewed Oct 31, 2024

View reviewed changes

michaelfig requested review from kriskowal, JeancarloBarrios and a team November 4, 2024 15:25

JeancarloBarrios reviewed Nov 4, 2024

View reviewed changes

gibson042 requested changes Nov 6, 2024

View reviewed changes

michaelfig requested a review from gibson042 November 6, 2024 23:57

gibson042 approved these changes Nov 7, 2024

View reviewed changes

feat(vtransfer): extract base address from parameterized address

3d44b53

michaelfig force-pushed the mfig-10249-vtransfer-virtual-targets branch from 6373538 to 3d44b53 Compare November 7, 2024 21:54

michaelfig added automerge:no-update (expert!) Automatically merge without updates and removed force:integration Force integration tests to run on PR labels Nov 7, 2024

mergify bot merged commit 4b1403a into master Nov 7, 2024
92 of 97 checks passed

mergify bot deleted the mfig-10249-vtransfer-virtual-targets branch November 7, 2024 22:10

turadg mentioned this pull request Dec 3, 2024

rework EUD address hook to use bech32 #10614

Closed

michaelfig mentioned this pull request Dec 8, 2024

feat: bech32 Address Hooks #10613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

michaelfig commented Oct 9, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 10, 2024 •

edited

Loading

LuqiPan left a comment

LuqiPan Oct 31, 2024

michaelfig Nov 1, 2024

LuqiPan Oct 31, 2024

michaelfig Nov 1, 2024

kriskowal Nov 5, 2024

JeancarloBarrios Nov 6, 2024

michaelfig Nov 6, 2024

JeancarloBarrios Nov 4, 2024

michaelfig Nov 5, 2024

gibson042 left a comment

gibson042 left a comment

gibson042 Nov 7, 2024

gibson042 Nov 7, 2024

gibson042 Nov 7, 2024

-// ExtractBaseTransferPacket returns the base address from the transfer packet's
-// transferData.Sender (if RoleSender) or transferData.Receiver (if
-// RoleReceiver). If newPacket is not nil, it is populated with a new transfer
-// packet whose corresponding Sender/Receiver is replaced with its base address.
-func ExtractBaseTransferPacket(cdc codec.Codec, packet ibcexported.PacketI, role AddressRole, newPacket *channeltypes.Packet) (string, error) {
+// 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) {

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

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

Conversation

michaelfig commented Oct 9, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Oct 10, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

LuqiPan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 left a comment

Choose a reason for hiding this comment

gibson042 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig commented Oct 9, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 10, 2024 •

edited

Loading