feat: `bech32` Address Hooks #10613

michaelfig · 2024-12-03T18:20:07Z

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

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.

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.

cloudflare-workers-and-pages · 2024-12-03T20:24:52Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`c8ad417`
Status:	✅ Deploy successful!
Preview URL:	https://03d783f9.agoric-sdk.pages.dev
Branch Preview URL:	https://mfig-bech32-address-hooks.agoric-sdk.pages.dev

View logs

dckc

I finished reviewing the JS part, which is handy, cuz now I grok the design...

dckc · 2024-12-10T19:23:03Z

packages/cosmic-proto/src/address-hooks.js

@@ -0,0 +1,231 @@
+/**
+ * @module address-hooks


nice docs. 👏

Bummer they don't show up in https://mfig-bech32-address-hooks.agoric-sdk.pages.dev/ 😢

Looks like cosmic-proto is entirely missing from our generated docs.

I wired up some docs now, which are better than nothing IMO. If they need much more work, I'd advocate for doing that in a new PR.

dckc · 2024-12-10T19:24:27Z

packages/cosmic-proto/src/address-hooks.js

+ *     const decoded = decodeAddressHook(addressHook);
+ *     // {
+ *     //   baseAddress: 'agoric1qqp0e5ys',
+ *     //   query: [Object: null prototype] { foo: [ 'bar', 'baz' ], key: 'value' }


supports array values. interesting.

cc @0xpatrickdev

dckc · 2024-12-10T19:26:26Z

packages/cosmic-proto/src/address-hooks.js

+export const DEFAULT_HOOKED_ADDRESS_CHAR_LIMIT = 1024;
+
+/**
+ * @typedef {Record<string, string | (string | null)[] | null>} HookQuery


NTS: What does null represent here, I wonder? Is this a convention from the query-string package?

I've documented the format in this JSDoc block... it is inherited from query-string, but I don't want to use query-string's types directly.

dckc · 2024-12-10T19:32:15Z

packages/cosmic-proto/src/address-hooks.js

+// The default maximum number of characters in a bech32-encoded hooked address.
+export const DEFAULT_HOOKED_ADDRESS_CHAR_LIMIT = 1024;


This is API documentation, not an implementation detail comment, right?

Suggested change

// The default maximum number of characters in a bech32-encoded hooked address.

export const DEFAULT_HOOKED_ADDRESS_CHAR_LIMIT = 1024;

/** The default maximum number of characters in a bech32-encoded hooked address. */

export const DEFAULT_HOOKED_ADDRESS_CHAR_LIMIT = 1024;

re Security Considerations, I see:

Advanced
BIP173 enforces a limitation of 90 characters, if extend the LIMIT parameter beyond this, be aware that the effectiveness of checksum decreases as the length increases.

I don't suppose we're relying on a bech32 checksum for security beyond helping detect typos, though.

That's right. The checksum is just to avoid typo footguns, not providing any kind of authentication.

dckc · 2024-12-10T19:42:38Z

packages/cosmic-proto/src/address-hooks.js

+
+  const magicLength = ADDRESS_HOOK_MAGIC.length;
+  const hookBuf = new Uint8Array(
+    magicLength + bytes.length + hookData.length + BASE_ADDRESS_LENGTH_BYTES,


Here we use hookData.length without any dynamic check on the type of hookData. So joinHookedAddress is exported but not as defensive as, say, ERTP stuff.

I wonder about documentation conventions for levels of defensiveness.

I've been living in a world of exo interface guards, so reviewing this sort of thing is slightly different.

I added some code to check that hookData.length is a non-negative safe integer. As for verifying the rest of its structure, I'll leave that up to hookBuf.set(hookData, ...).

dckc · 2024-12-10T20:16:55Z

packages/cosmic-proto/test/address-hooks.test.js

+test(
+  'roundtripEmpty',


after mulling over what this test establishes, I thought about property testing / fuzzing to be sure the encoding is unambiguous - that no 2 distinct inputs lead to the same encoded address.

But that would say that the design is flawed. And as noted above, I assume the design is agreed by the relevant parties.

I'm not entirely enamoured with the tests as they exist now. They're that way by attrition, less so by intention.

And they don't cover any of the error cases, just manual happy paths.

dckc · 2024-12-10T20:25:33Z

packages/cosmic-proto/test/address-hooks.test.js

+    dst: ['a', 'b', 'c'],
+  },
+  'cosmos10rchqqplv3ehg0tpyej8xapavgnxgum5843jvetkv4e8jargd9hxwqqp4vx73n',
+);


To check default length limits against expected fast-usdc usage, let's please add a test to replace what's currently in the fast usdc test fixture: agoric16kv2g7snfc4q24vg3pjdlnnqgngtjpwtetd2h689nz09lcklvh5s8u37ek?EUD=osmo183dejcnmkka5dzcu9xw6mywq0p2m5peks28men

base address is agoric16kv2g7snfc4q24vg3pjdlnnqgngtjpwtetd2h689nz09lcklvh5s8u37ek

query is { EUD: 'osmo183dejcnmkka5dzcu9xw6mywq0p2m5peks28men' }

Encoded as agoric10rchp4vc53apxn32q42c3zryml8xq3xshyzuhjk6405wtxy7tl3d7e0f8az423padaek6me38qekget2vdhx66mtvy6kg7nrw5uhsaekd4uhwufswqex6dtsv44hxv3cd4jkuqpqvduyhf

Length: 149

We're mostly paying the 8/5 bech32 encoding tax on every byte of the query string. If we really need a smaller string, we'd have to resort to adding some compression.

dckc · 2024-12-10T20:28:47Z

packages/cosmic-proto/package.json

+    "bech32": "^2.0.0",
+    "query-string": "^9.1.1"


supply chain audit notes: I don't see any yarn.lock changes re bech32, so I'll pass on that without further remark.

query-string@^9.1.1 seems to be new. npm shows 13M weekly downloads. Clearly pretty mature stuff.

dckc · 2024-12-10T20:30:23Z

packages/cosmic-proto/test/address-hooks.test.js

@@ -0,0 +1,214 @@
+import rawTest from '@endo/ses-ava/prepare-endo.js';


Sometimes I wonder whether I should be using ses-ava. For my edification: what motivated you to use it here?

It was available from @endo, so it didn't cause dependency cycles like @agoric/swingset-vat/tools/prepare-test-env-ava.js would.

dckc · 2024-12-10T20:31:39Z

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

+	"github.com/Agoric/agoric-sdk/golang/cosmos/types"
 	"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"
-	"github.com/Agoric/agoric-sdk/golang/cosmos/x/vtransfer/types"
+


I don't know how to read this change. What does it do?

I moved .../x/vtransfer/types/baseaddr.go to .../types/address_hooks.go. This change gets the types.Whatever functions (whose callers were unmodified in this PR, so they don't show up in its commits) from the new Golang package (stuff in a single directory) where I moved address_hooks.go.

dckc

dealing with byte slices and such is quite a bit nicer in go than JS, isn't it :)

Before I approve, I think I should understand the change in golang/cosmos/x/vtransfer/keeper/keeper.go.

dckc · 2024-12-10T21:40:42Z

golang/cosmos/types/address_hooks.go

+func init() {
+	if AddressHookVersion&0x0f != AddressHookVersion {
+		panic(fmt.Sprintf("AddressHookVersion must be less than 0x10, got 0x%x", AddressHookVersion))
+	}
 }

 // ExtractBaseAddress extracts the base address from a parameterized address.
 // It removes all subpath and query components from addr.


subpath... I don't remember that in the JS stuff.

It's a stale comment. In the new encoding, there is never a "subpath" since the baseAddr isn't actually in the same string as the query.

dckc · 2024-12-10T21:56:18Z

golang/cosmos/types/address_hooks.go

-		// Skip over parsing control flags.
-		ForceQuery: parsed.ForceQuery,
-		OmitHost:   parsed.OmitHost,
+	bz := bytes.TrimPrefix(payload, AddressHookMagic)


bz is mnemonic for something like "bytez", a play on "bytes"?

Yes, it's a Golang convention I picked up from Golang json and protobuf codecs.

dckc · 2024-12-10T21:57:01Z

golang/cosmos/types/address_hooks.go

-		return "", fmt.Errorf("base address cannot be empty")
+	b := 0
+	for i := BaseAddressLengthBytes - 1; i >= 0; i -= 1 {
+		by := bz[len(bz)-1-i]


now we have by and bz, as if it were an x, y, z thing. But z is a slice and y is a byte. ouch. hurts my brain.

I can't call it byte because that's a builtin data type. I'll try byteVal.

dckc · 2024-12-10T22:01:58Z

golang/cosmos/types/address_hooks.go

+	}
+
+	b := len(bz)
+	if b > 0xffff {


elsewhere we act like the number of bytes in a base address length is a parameter, and we do loops and stuff. Why not hard-code 2 bytes everywhere?

Thanks for pointing this out. I think this was half-finished as I rewrote the JS encoder (in my first upgrade from 1 length byte to 2 length bytes).

I want to leave it as a parameter, so I did that.

dckc · 2024-12-10T22:03:13Z

golang/cosmos/types/address_hooks.go

+	payload = append(payload, AddressHookMagic...)
+	payload = append(payload, bz...)
+	payload = append(payload, hookData...)
+	payload = append(payload, byte(b>>8), byte(b))


another place where BaseAddressLengthBytes has to be 2.

dckc · 2024-12-10T22:05:20Z

golang/cosmos/types/address_hooks_test.go

 	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},
+		{"agoric address", "agoric1qqp0e5ys"},
+		{"cosmos address", "cosmos1qqxuevtt"},


go has nice table literals!

dckc · 2024-12-10T22:09:31Z

golang/cosmos/types/address_hooks_test.go

+	cosmosHook, err := types.JoinHookedAddress("cosmos1qqxuevtt", []byte("?foo=bar&baz=bot#fragment"))
+	require.NoError(t, err)


This seems to test only that the call does not err. Is that right?

I'm curious what happens to #fragment.

I tested SplitHookedAddress and verified the results. That turned up a bug (wasn't stripping the baseAddr length bytes from the end of an Address Hook). Thanks!

mergify · 2024-12-11T00:47:49Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ integration-test-result
☑️ no-fixup-commits

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

gibson042

Looks like I didn't finish reviewing before this was merged, but I think it's a bad idea for these implementation to pass through hooked addresses with nonzero version nibble as if they were not hooked. Instead, such addresses should be rejected for using an unsupported version, allowing us to actually introduce such functionality in the future.

gibson042 · 2024-12-11T06:02:04Z

golang/cosmos/types/address_hooks.go

+	bz := bytes.TrimPrefix(payload, AddressHookMagic)
+	if len(bz) == len(payload) {
+		// Return an unhooked address.
+		return addr, []byte{}, nil
+	}


For forward compatibility, this should probably error upon encountering a hooked address with an unknown version.

Suggested change

bz := bytes.TrimPrefix(payload, AddressHookMagic)

if len(bz) == len(payload) {

// Return an unhooked address.

return addr, []byte{}, nil

}

bz := bytes.TrimPrefix(payload, AddressHookMagic[:2])

if len(bz) == len(payload) || len(bz) == 0 || bz[0] & 0x70 != 0x70 {

// Return an unhooked address.

return addr, []byte{}, nil

}

version := bz[0] & 0x0F

bz = bz[1:]

if version != AddressHookVersion {

return "", []byte{}, fmt.Errorf("unsupported version %d", version)

}

(and likewise in the JS version)

@turadg , @LuqiPan ,

I suppose this shows that my assumption that the design was agreed by all the relevant parties is not correct.

turadg mentioned this pull request Dec 3, 2024

rework EUD address hook to use bech32 #10614

Closed

michaelfig force-pushed the mfig-bech32-address-hooks branch from 4525767 to 649b6e9 Compare December 3, 2024 20:24

michaelfig force-pushed the mfig-bech32-address-hooks branch 3 times, most recently from 5255c1a to 2083b95 Compare December 8, 2024 01:29

michaelfig marked this pull request as ready for review December 8, 2024 01:52

michaelfig requested a review from a team as a code owner December 8, 2024 01:52

michaelfig requested a review from AgoricTriage December 8, 2024 01:52

michaelfig self-assigned this Dec 8, 2024

michaelfig requested review from turadg and gibson042 December 8, 2024 02:06

michaelfig force-pushed the mfig-bech32-address-hooks branch 2 times, most recently from 7f36f5d to a7fb069 Compare December 9, 2024 00:52

0xpatrickdev mentioned this pull request Dec 9, 2024

FU multichain testing of happy path #10597

Closed

3 tasks

michaelfig force-pushed the mfig-bech32-address-hooks branch 3 times, most recently from b85e8fc to 17cbf36 Compare December 10, 2024 17:56

michaelfig marked this pull request as draft December 10, 2024 18:02

michaelfig force-pushed the mfig-bech32-address-hooks branch from 17cbf36 to 40191ad Compare December 10, 2024 18:07

LuqiPan requested a review from dckc December 10, 2024 18:21

michaelfig force-pushed the mfig-bech32-address-hooks branch from 40191ad to 77f9587 Compare December 10, 2024 18:32

michaelfig marked this pull request as ready for review December 10, 2024 18:32

dckc reviewed Dec 10, 2024

View reviewed changes

dckc approved these changes Dec 11, 2024

View reviewed changes

michaelfig force-pushed the mfig-bech32-address-hooks branch from 5023e23 to f984698 Compare December 11, 2024 00:09

michaelfig added automerge:no-update (expert!) Automatically merge without updates agd Agoric (Golang) Daemon labels Dec 11, 2024

michaelfig force-pushed the mfig-bech32-address-hooks branch from f984698 to 6ff495a Compare December 11, 2024 01:42

michaelfig added 4 commits December 10, 2024 20:07

feat(vats): first cut of Address Hooks in JS

dbad30b

build(deps): regenerate yarn.locks

5014cf9

feat(vtransfer): port some address-hooks.js functions to Go

159098b

docs(cosmic-proto): generate initial typedoc

c8ad417

michaelfig force-pushed the mfig-bech32-address-hooks branch from 6ff495a to c8ad417 Compare December 11, 2024 02:07

mergify bot merged commit 945fef1 into master Dec 11, 2024
81 checks passed

mergify bot deleted the mfig-bech32-address-hooks branch December 11, 2024 02:55

gibson042 reviewed Dec 11, 2024

View reviewed changes

michaelfig mentioned this pull request Dec 11, 2024

Address Hooks are misclassified if versions don't match #10681

Closed

		// The default maximum number of characters in a bech32-encoded hooked address.
		export const DEFAULT_HOOKED_ADDRESS_CHAR_LIMIT = 1024;

		@@ -0,0 +1,214 @@
		import rawTest from '@endo/ses-ava/prepare-endo.js';

		cosmosHook, err := types.JoinHookedAddress("cosmos1qqxuevtt", []byte("?foo=bar&baz=bot#fragment"))
		require.NoError(t, err)

		test(
		'roundtripEmpty',

feat: bech32 Address Hooks #10613

feat: bech32 Address Hooks #10613

Conversation

michaelfig commented Dec 3, 2024 • edited Loading

Description

Example

Encoding

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Dec 3, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

dckc 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

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

Choose a reason for hiding this comment

dckc 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

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

mergify bot commented Dec 11, 2024

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

feat: `bech32` Address Hooks #10613

feat: `bech32` Address Hooks #10613

michaelfig commented Dec 3, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 3, 2024 •

edited

Loading