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(orchestration): chain NameHub #9323

Closed
wants to merge 2 commits into from
Closed

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented May 6, 2024

closes: #XXXX
refs: #9063
refs: #9211
refs: #8879

Description

Seeking early feedback on the approach for a chain registry to facilitate message building for orchestration api consumers.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

@0xpatrickdev 0xpatrickdev changed the title test(orchestration): chain NameHub feat(orchestration): chain NameHub May 6, 2024
@0xpatrickdev
Copy link
Member Author

0xpatrickdev commented May 6, 2024

Here's what packages/orchestration/src/types.d.ts might look like after this change. Summary:

  • connectionInfo must be a list - each chain has multiple connections. Now accessible via connection, keyed by AllegedChainName (like AllegedBrandName)
  • adds lookup methods for denom and brand (trace from the PR is not included)
  • does not model ICA/ICQ/PFM features - not sure these are needed anymore. When would a contract dev need this at runtime?
  • updates NameHub to take generic type parameters for key and value
git diff --staged packages/orchestration/src/types.d.ts
diff --git a/packages/orchestration/src/types.d.ts b/packages/orchestration/src/types.d.ts
index 13e457f29..a21abf697 100644
--- a/packages/orchestration/src/types.d.ts
+++ b/packages/orchestration/src/types.d.ts
@@ -19,8 +19,9 @@ import type {
 } from '@agoric/vats/tools/ibc-utils.js';
 import type { Port } from '@agoric/network';
 import { MsgTransferResponse } from '@agoric/cosmic-proto/ibc/applications/transfer/v1/tx.js';
-import type { IBCConnectionID } from '@agoric/vats';
+import type { IBCConnectionID, NameHub } from '@agoric/vats';
 import type { ICQConnection } from './exos/icqConnectionKit.js';
+import { IBCDenom } from '../test/test-chain-name-hub.js';

 export type * from './service.js';
 export type * from './vat-orchestration.js';
@@ -172,25 +173,31 @@ export type EthChainInfo = {
   allegedName: string;
 };

+type IBCConnectionInfo = {
+  id: string; // e.g. connection-0
+  client_id: string; // '07-tendermint-0'
+  state: 'OPEN' | 'TRYOPEN' | 'INIT' | 'CLOSED';
+  counterparty: {
+    client_id: string;
+    connection_id: string;
+    prefix: {
+      key_prefix: string;
+    };
+  };
+  versions: { identifier: string; features: string[] }[];
+  delay_period: bigint;
+};
+
+// make this a brand?
+type AllegedChainName = string;
 /**
  * Info for a Cosmos-based chain.
  */
 export type CosmosChainInfo = {
   chainId: string;
-  ibcConnectionInfo: {
-    id: string; // e.g. connection-0
-    client_id: string; // '07-tendermint-0'
-    state: 'OPEN' | 'TRYOPEN' | 'INIT' | 'CLOSED';
-    counterparty: {
-      client_id: string;
-      connection_id: string;
-      prefix: {
-        key_prefix: string;
-      };
-    };
-    versions: { identifier: string; features: string[] }[];
-    delay_period: bigint;
-  };
+  connection: NameHub<AllegedChainName, IBCConnectionInfo>;
+  denom: NameHub<ReturnType<Brand['getAllegedName']>, IBCDenom>;
+  brand: NameHumb<IBCDenom, Brand>;
   icaEnabled: boolean;
   icqEnabled: boolean;
   pfmEnabled: boolean;
diff --git a/packages/vats/src/types.d.ts b/packages/vats/src/types.d.ts
index 1f33d7c65..0361290b4 100644
--- a/packages/vats/src/types.d.ts
+++ b/packages/vats/src/types.d.ts
@@ -9,19 +9,19 @@ export type Board = ReturnType<
  * allow passing a remote iterable, there would be an inordinate number of round
  * trips for the contents of even the simplest nameHub.
  */
-export type NameHub = {
-  has: (key: string) => boolean;
+export type NameHub<K extends string = string, V = unknown> = {
+  has: (key: K) => boolean;
   /**
    * Look up a path of keys starting from the current NameHub. Wait on any
    * reserved promises.
    */
-  lookup: (...path: string[]) => Promise<any>;
+  lookup: (...path: K[]) => Promise<any>;
   /** get all the entries available in the current NameHub */
-  entries: (includeReserved?: boolean) => [string, unknown][];
+  entries: (includeReserved?: boolean) => [K, V][];
   /** get all names available in the current NameHub */
-  keys: () => string[];
+  keys: () => K[];
   /** get all values available in the current NameHub */
-  values: () => unknown[];
+  values: () => V[];
 };

* [denom: IBCDenom]: Brand;
* };
* denom: {
* [allegedBrandName: string]: IBCDenom;
Copy link
Member

Choose a reason for hiding this comment

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

alleged names should only be used for debugging, not for anything that affects correctness

Copy link
Member Author

@0xpatrickdev 0xpatrickdev May 6, 2024

Choose a reason for hiding this comment

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

What is the correct name/type for FOO when we do E(agoricNames).lookup('brand', 'FOO')?

Copy link
Member

Choose a reason for hiding this comment

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

issuerName, in the case of brands

more generally, just name (or key)

* };
* };
* connection: {
* [allegedChainName: string]: {
Copy link
Member

Choose a reason for hiding this comment

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

alleged names should not be used for anything that affects correctness

const { nameHub: chainNames, nameAdmin: chainNamesAdmin } = makeNameHubKit();
for (const [allegedChainName, value] of Object.entries(mockChainInfo)) {
const { bondDenom, chainId, denoms, connections } = value;
const child = await chainNamesAdmin.provideChild(allegedChainName, [
Copy link
Member

Choose a reason for hiding this comment

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

alleged names should not be used for anything that affects correctness

Comment on lines +54 to +55
* brand: {
* [denom: IBCDenom]: Brand;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see Brands as intrinsic to chains.
The existing VBankAsset mapping from denoms to brands seems to suffice.

baseDenom: 'uosmo',
denom:
'ibc/ED07A3391A112B175915CD8FAF43A2DA8E4790EDE12566649D0C2F97716B8518',
path: 'transfer/channel-0',
Copy link
Member

Choose a reason for hiding this comment

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

channel 0 goes to the cosmos hub, according to https://mapofzones.com/zones/agoric-3/peers?columnKey=ibcVolumeIn&period=24h

or is this about emerynet or something?

Comment on lines +63 to +64
lastUpdated: {
revisionHeight: 831n,
Copy link
Member

Choose a reason for hiding this comment

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

how often do you expect this to get updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty frequently, I believe with every block (of the chain with the shorter avg block time).

I recognize this likely makes it a poor candidate to put into state, given our anticipated update frequency/governance, but some messages seem like they rely on it for preventing an unbound timeout:

  1. MsgTransfer has timeoutHeight and timeoutTimestamp fields (the generated TS types say both are required, but i think it's actually one or the other)
  2. RequestQuery has a height field used for timing out a packet
  3. TxBody has a timeoutHeight field used for timing out a packet

More on Height:

/**
 * Height is a monotonically increasing data type
 * that can be compared against another Height for the purposes of updating and
 * freezing clients
 *
 * Normally the RevisionHeight is incremented at each height while keeping
 * RevisionNumber the same. However some consensus algorithms may choose to
 * reset the height in certain conditions e.g. hard forks, state-machine
 * breaking changes In these cases, the RevisionNumber is incremented so that
 * height continues to be monitonically increasing even as the RevisionHeight
 * gets reset
 */
export interface Height {
    /** the revision that the client is currently on */
    revisionNumber: bigint;
    /** the height within the given revision */
    revisionHeight: bigint;
}

For MsgTransfer, it seems that we should use timeoutTimestamp (absolute) to set the timeout, leaning on TimerService.

For RequestQuery and TxBody, maybe the correct abstraction for now is to ensure timeoutHeight/height is always an (invitation|offer)Arg, and rely on clients for this information.

A clever approach might be to approximate timeout height based on a target block duration (of the chain with the shorter avg block times), using using a historical revisionHeight and the timestamp is was saved at. This is likely too clever, and doesn't account for things like chain halting upgrades or target block time drift.

P.S. created #9324 to capture this work, as TxBody's revisionHeight and RequestQuery's height default to 0n on master

Copy link

cloudflare-workers-and-pages bot commented May 6, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: bb34c13
Status: ✅  Deploy successful!
Preview URL: https://8eaebc1d.agoric-sdk.pages.dev
Branch Preview URL: https://9063-well-known-chains.agoric-sdk.pages.dev

View logs

'trace',
'connection',
]);
child.nameAdmin.update('chainId', chainId);
Copy link
Member

Choose a reason for hiding this comment

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

why should the info about a chain be mutable?
why not just store a big record / structure, as is done in vbankAsset?

const bondAsset = await chainNames.lookup('osmosis', 'bondAsset');
t.deepEqual(bondAsset, { brand: 'XXX_agoricNames_brand', denom: 'uosmo' });

// lookup up Brand from denom
Copy link
Member

Choose a reason for hiding this comment

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

That's what vbankAsset is for, no?

To look up uosmo, you'd have to know the channel to compute the relevant ibc denom first, of course.

t.deepEqual(traceFromBrandName, expectedTrace);

// lookup conneciton info for a chain
const connection = await chainNames.lookup('osmosis', 'connection', 'agoric');
Copy link
Member

Choose a reason for hiding this comment

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

I don't expect oasis to decide what the name "agoric" means.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need some mechanism to reference a particular Connection in osmosis's list of connections. chainId: string was a candidate, but I went with ChainName (AllegedChainName in the current revision of the PR , but intended to be chainName, like issuerName)

Copy link
Member

Choose a reason for hiding this comment

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

We need some mechanism to reference a particular Connection ...

connection id doesn't suffice for some reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think one of the main reasons to query the 'connection' namespace is to determine the connectionId (from both the host + remote's perspectives if creating an ICA). The other would be to get channelIds of the ics20-1 channel for MsgTransfers

Copy link
Member

Choose a reason for hiding this comment

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

I suspect I would prefer using reverse-lookup for some of this.

Here's hoping to think this over in more detail shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants