feat(orchestration): lookup `ChainInfo` from address value #10354

0xpatrickdev · 2024-10-28T22:35:41Z

Description

add bech32Prefix?: string to CosmosChainInfo and populate data with yarn codegen
add getChainInfoByAddress lookup method to ChainHub

Security Considerations

If multiple cosmos chains use the same bech32Prefix, this approach is not sound. In practice, this doesn't seem to be an issue. Developers also control their own ChainHubs and choose whether they want to populate it with data from agoricNames. The potential impact is loss of funds from a misrouted transfer.

Scaling Considerations

Adds another exo to keep track of bech32Prefix:chainName mapping.
Adds more data to agoricNames (negligible)

Documentation Considerations

Typedoc will include these changes

Testing Considerations

Includes unit tests and updates snapshot tests

Upgrade Considerations

Library code, part of an NPM orch release.

cloudflare-workers-and-pages · 2024-10-28T22:38:39Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`8ba4699`
Status:	✅ Deploy successful!
Preview URL:	https://e1c8bb50.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-bech-32-prefix.agoric-sdk.pages.dev

View logs

packages/orchestration/src/fetched-chain-info.js

dckc

Strictly speaking, lastIndexOf is a correctness issue. I suppose it deserves a test.

factoring out getBech32Prefix(addr) is a suggestion; deal with it at your discretion.

dckc · 2024-10-29T17:15:38Z

packages/orchestration/src/exos/chain-hub.js

@@ -216,6 +217,11 @@ export const makeChainHub = (zone, agoricNames, vowTools) => {
    keyShape: BrandShape,
    valueShape: M.string(),
  });
+  /** @type {MapStore<string, string>} */
+  const bech32PrefixToChainName = zone.mapStore('bech32PrefixToChainName', {


The 1st arg to makeScalarMapStore is a tag for just the key, not the whole map, for use in error messages such as bech32Prefix not found: blort. So I was going to suggest:

Suggested change

const bech32PrefixToChainName = zone.mapStore('bech32PrefixToChainName', {

const bech32PrefixToChainName = zone.mapStore('bech32Prefix', {

but it looks like the arg here is a baggage key, so the longer name seems right.

dckc · 2024-10-29T17:22:55Z

packages/orchestration/src/exos/chain-hub.js

+      const [prefix, rest] = address.split('1');
+      (prefix && rest) ||
+        Fail`Address contains invalid bech32 separator: ${q(address)}`;


cosmjs uses bech32, which uses lastIndexOf, not the 1st occurrence.

https://github.com/bitcoinjs/bech32/blob/5ceb0e3d4625561a459c85643ca6947739b2d83c/src/index.ts#L146

I wonder about factoring out a getBech32Prefix(addr) with its own unit tests.

the so-called reference implementation uses .rfind('1') too.

https://github.com/sipa/bech32/blob/7a7d7ab158db7078a333384e0e918c90dbc42917/ref/python/segwit_addr.py#L79

Thanks - glad I tagged you as a reviewer! Will factor this out to its own function and adopt the new logic.

Interestingly, in practice no one seems to use 1 in the prefix / human readable part:
https://github.com/satoshilabs/slips/blob/master/slip-0173.md#registered-human-readable-parts

But the spec does say any US-ASCII character, in the range [33-126], is valid for the HRP:
https://github.com/bitcoin/bips/blob/7a104491862379e88662a39d1a2b613c907e503e/bip-0173.mediawiki#bech32

Please see the amended commit: 65e07f8

dckc · 2024-10-29T17:23:53Z

packages/orchestration/src/exos/chain-hub.js

+     * @param {string} address bech32 address
+     * @returns {CosmosChainInfo}
+     */
+    getChainInfoFromAddress(address) {


Did you consider getChainInfoFromPrefix(getBech32Prefix(address)) instead or in addition?

I didn't, no.

in addition

I think we should only have one function for this to reduce info overload.

instead

I still feel getChainInfoFromAddress is the sensible abstraction, but happy to refactor to getChainInfoFromPrefix if you and other reviewers are in agreement that its preferable.

This starts to look like a product question. getChainInfoFromAddress is what #10349 calls for, and I guess it got an OK from product.

I don't feel strongly about it. I guess getChainInfoFromAddress is fine by me.

For transparency, I wrote the ticket and prescribed the name there. I don't think product typically weighs in on function names, just the user story (here: "look up chainId from chain address value").

dckc · 2024-10-30T15:47:05Z

packages/orchestration/test/utils/address.test.ts

+      specimen: '111qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4',
+      expected: '11',


👏

This test distinguishes the correct .lastIndexOf() implementation from the indexOf() style.

turadg · 2024-10-30T17:49:22Z

packages/orchestration/src/exos/chain-hub.js

+      if (bech32PrefixToChainName.has(prefix)) {
+        const chainName = bech32PrefixToChainName.get(prefix);
+        return chainInfos.get(chainName);
+      }
+      throw makeError(`Chain info not found for bech32Prefix ${q(prefix)}`);


consider: generally better to validate before getting to the happy path:

Suggested change

if (bech32PrefixToChainName.has(prefix)) {

const chainName = bech32PrefixToChainName.get(prefix);

return chainInfos.get(chainName);

}

throw makeError(`Chain info not found for bech32Prefix ${q(prefix)}`);

if (!bech32PrefixToChainName.has(prefix)) {

throw makeError(`Chain info not found for bech32Prefix ${q(prefix)}`);

}

const chainName = bech32PrefixToChainName.get(prefix);

return chainInfos.get(chainName);

turadg · 2024-10-30T17:52:43Z

packages/orchestration/test/exos/chain-hub.test.ts

@@ -177,3 +159,35 @@ test('toward asset info in agoricNames (#9572)', async t => {
    });
  }
 });
+
+test.serial('getChainInfoByAddress', async t => {


why is this serial now? each test has its own setup.

we should avoid serial when we can

Without this, and the addition of the getChainInfoByAddress test, toward asset info in agoricNames (#9572) starts failing with:

SES_UNHANDLED_REJECTION: (Error#1) Error#1: promise watcher must be a virtual object at makeError (file:///Users/0xpatrick/repos/agoric-sdk/node_modules/ses/src/error/assert.js:350:61) at fail (file:///Users/0xpatrick/repos/agoric-sdk/node_modules/ses/src/error/assert.js:482:20) at Fail (file:///Users/0xpatrick/repos/agoric-sdk/node_modules/ses/src/error/assert.js:492:39) at file:///Users/0xpatrick/repos/agoric-sdk/packages/swingset-liveslots/src/watchedPromises.js:216:13 at process.processTicksAndRejections (node:internal/process/task_queues:95:5) ─ toward asset info in agoricNames (#9572) Error: Promise returned by test never resolved

Ok, out of scope. Turns out provideDurableZone mutates global state. I'll look into it

turadg · 2024-10-30T17:55:20Z

packages/orchestration/test/utils/address.test.ts

+    },
+  ];
+
+  for (const { specimen, expected, error } of testCases) {


consider an ava macro (e.g. bech32) so you can have a test for each case:

test(bech32, 'bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4', 'bc'); … test(bech32, '1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4', null, 'Missing prefix for "1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4"');

- takes a ChainAddress.value, parses the bech32Prefix, and looks up the corresponding ChainInfo

- prevents empty entries from affecting sort order and causing unnecessary git diff churn

0xpatrickdev requested a review from a team as a code owner October 28, 2024 22:35

0xpatrickdev requested a review from AgoricTriage October 28, 2024 22:35

0xpatrickdev force-pushed the pc/bech-32-prefix branch from 5e83e42 to bbcc8d7 Compare October 28, 2024 22:46

0xpatrickdev commented Oct 28, 2024

View reviewed changes

packages/orchestration/src/fetched-chain-info.js Outdated Show resolved Hide resolved

0xpatrickdev force-pushed the pc/bech-32-prefix branch 2 times, most recently from cf708f8 to 832d26f Compare October 29, 2024 00:37

0xpatrickdev requested review from turadg, dckc and iomekam October 29, 2024 00:50

0xpatrickdev force-pushed the pc/bech-32-prefix branch 2 times, most recently from 4ad588d to 33b5f80 Compare October 29, 2024 14:36

LuqiPan assigned iomekam Oct 29, 2024

dckc requested changes Oct 29, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/bech-32-prefix branch 2 times, most recently from c832f82 to 28bbace Compare October 30, 2024 15:14

0xpatrickdev requested a review from dckc October 30, 2024 15:16

dckc approved these changes Oct 30, 2024

View reviewed changes

0xpatrickdev unassigned iomekam Oct 30, 2024

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Oct 30, 2024

0xpatrickdev force-pushed the pc/bech-32-prefix branch from 28bbace to 9613b10 Compare October 30, 2024 17:02

0xpatrickdev removed the automerge:rebase Automatically rebase updates, then merge label Oct 30, 2024

0xpatrickdev force-pushed the pc/bech-32-prefix branch 2 times, most recently from 565ad9a to c94f5fb Compare October 30, 2024 17:48

turadg reviewed Oct 30, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/bech-32-prefix branch 3 times, most recently from 6c9e11d to 08ed142 Compare October 30, 2024 18:57

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Oct 31, 2024

0xpatrickdev force-pushed the pc/bech-32-prefix branch from 08ed142 to 9ec159c Compare October 31, 2024 22:47

0xpatrickdev added 4 commits October 31, 2024 23:27

feat: add bech32Prefix?: string to CosmosChainInfo

cb9e1ee

chore: remove dead test code

6b3d31a

feat(chainHub): getChainInfoByAddress helper

d6c487c

- takes a ChainAddress.value, parses the bech32Prefix, and looks up the corresponding ChainInfo

fix: convertChainInfo connection sorting

8ba4699

- prevents empty entries from affecting sort order and causing unnecessary git diff churn

0xpatrickdev force-pushed the pc/bech-32-prefix branch from 9ec159c to 8ba4699 Compare October 31, 2024 23:27

mergify bot merged commit aba3e48 into master Nov 1, 2024
80 checks passed

mergify bot deleted the pc/bech-32-prefix branch November 1, 2024 00:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(orchestration): lookup `ChainInfo` from address value #10354

feat(orchestration): lookup `ChainInfo` from address value #10354

0xpatrickdev commented Oct 28, 2024 •

edited

Loading

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

edited

Loading

dckc left a comment

dckc Oct 29, 2024

dckc Oct 29, 2024

dckc Oct 29, 2024

0xpatrickdev Oct 29, 2024

0xpatrickdev Oct 30, 2024 •

edited

Loading

dckc Oct 29, 2024

0xpatrickdev Oct 29, 2024

dckc Oct 29, 2024

0xpatrickdev Oct 30, 2024

dckc Oct 30, 2024

turadg Oct 30, 2024

turadg Oct 30, 2024

0xpatrickdev Oct 30, 2024

turadg Oct 30, 2024

turadg Oct 30, 2024

	const bech32PrefixToChainName = zone.mapStore('bech32PrefixToChainName', {
	const bech32PrefixToChainName = zone.mapStore('bech32Prefix', {

		specimen: '111qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4',
		expected: '11',

feat(orchestration): lookup ChainInfo from address value #10354

feat(orchestration): lookup ChainInfo from address value #10354

Conversation

0xpatrickdev commented Oct 28, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Oct 28, 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

0xpatrickdev Oct 30, 2024 • edited Loading

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

feat(orchestration): lookup `ChainInfo` from address value #10354

feat(orchestration): lookup `ChainInfo` from address value #10354

0xpatrickdev commented Oct 28, 2024 •

edited

Loading

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

edited

Loading

0xpatrickdev Oct 30, 2024 •

edited

Loading