feat(cosmosStakingAccount): publish chain address to vstorage #9482

0xpatrickdev · 2024-06-11T15:11:00Z

Description

Motivation: wallet clients need a way to view their address so they can send funds to it
Updates stakeAtom.contract.js to create a vstorage entry for newly created accounts, keyed by ChainAccount['address'] (e.g. cosmos1testing1234)
- TODO: if we hit UNPARSABLE_ADDRESS in parseAddress we should throw, or use a fallback value
Writes an empty string to provided storageNode in exos/cosmosOrchestrationAccount.js
- adds TODO for fleshing out vstorage requirements via orchestration - ICA Controller - Account - vstorage logging #9066
Drive-by fixes:
- use AmountArg consistently in /exos/cosmosOrchestrationAccount.js
- remove delegatorAddress from undelegate(Delegations[]) interface and implementation

Security Considerations

Scaling Considerations

This is some experimentation within a contract in order to get experience with what should be pushed down into the platform.

The approach delegates vstorage publishing responsibilities to guest contracts. We might consider posting information for all accounts to a shared global namespace.

Documentation Considerations

Testing Considerations

Tests, including ones for swapExample.contract.js and facade.js, were updated to accommodate these changes. Mocking facilities for .getAddress() are light - including in boostrap tests - so using ChainAddress['address'] might lead to unexpected behavior (accounts overwriting each other in storage) in a testing environment.

Upgrade Considerations

0xpatrickdev · 2024-06-11T15:14:26Z

packages/orchestration/test/examples/stake-atom.contract.test.ts

+
+  // FIXME mock remoteAddress in ibc bridge
+  const storagePath =
+    'mockChainStorageRoot.stakeAtom.accounts.UNPARSABLE_CHAIN_ADDRESS';


We might consider throwing if the address is unparsable, or falling back to a different identifier like Port Address + ControllerConnectionID: icacontroller-1-connection-0

Not sure the solution but we definitely shouldn't write UNPARSABLE_CHAIN_ADDRESS to vstorage

I added a FIXME to chainAccount.js with a link to #9066

0xpatrickdev · 2024-06-11T15:16:56Z

packages/orchestration/src/exos/cosmosOrchestrationAccount.js

@@ -158,6 +158,7 @@ export const prepareCosmosOrchestrationAccountKit = (
      // must be the fully synchronous maker because the kit is held in durable state
      // @ts-expect-error XXX Patterns
      const topicKit = makeRecorderKit(storageNode, PUBLIC_TOPICS.account[1]);
+      void E(topicKit.recorder).write(harden({ sequence: 0n }));


I've selected sequence since it seems we need to create an initial write. I considered the ChainAddress object but that didn't seem to make sense when we're keying by ChainAddress['address'].

If folks think storing sequence makes sense, we can increment this after each successful executeEncodedTx.

It seems useful but it does have a cost. Is it just helpful for diagnostics or is there a product use case to publish each sequence?

If we do include it, consider a more transparent property name like executedSequence

I removed for now in favor of an empty string ''. Will wait for use cases.

cloudflare-workers-and-pages · 2024-06-11T16:17:46Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`11aea65`
Status:	✅ Deploy successful!
Preview URL:	https://215182fe.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-9042-stake-atom-vstorage.agoric-sdk.pages.dev

View logs

turadg

This is some experimentation within a contract in order to get experience with what should be pushed down into the platform.

Very well!

The approach delegates vstorage publishing responsibilities to guest contracts.

I agree that's appropriate at this point and expect we'll develop the abstractions for cleaner start functions after we've got most of the pieces working end to end.

I left some suggestions but no blockers. I did have some questions so I'm holding Approve for those.

turadg · 2024-06-13T13:48:17Z

packages/orchestration/src/examples/stakeIca.contract.js

+  const { accountsStorageNode } = await provideAll(baggage, {
+    accountsStorageNode: () => E(storageNode).makeChildNode('accounts'),


I take it you say accountNode below but StorageNode here because it's going in baggage. Consider noting that rationale since this is an example contract.

turadg · 2024-06-13T13:53:34Z

packages/orchestration/src/examples/swapExample.contract.js

+  const { accountsStorageNode } = await provideAll(baggage, {
+    accountsStorageNode: () => E(storageNode).makeChildNode('accounts'),
+  });


What's the thought behind the parent of orchestration being accounts? It will need to put other info in there, won't it?

I won't block on this since the code is still experimental and we have #9066 ahead of us. But consider makeChildNode('orchestration') since that's the scope it's being handed to.

Suggested change

const { accountsStorageNode } = await provideAll(baggage, {

accountsStorageNode: () => E(storageNode).makeChildNode('accounts'),

});

const { orchestrationStorageNode } = await provideAll(baggage, {

orchestrationStorageNode: () => E(storageNode).makeChildNode('orchestration'),

});

We should try to be consistent across contracts so that we can have a helper that does all this setup in one call from start.

We make separate nodes for stakeAtom and stakeOsmo in the start-*.js scripts, and each has an accounts child node. Presumably other info or child nodes can be written to the stakeAtom and stakeOsmo paths.

This seems to be in line with "delegates vstorage publishing responsibilities to guest contracts" in the PR description.

Agree it may make sense to globally publish something for all accounts, unless explicitly opted out, and that's where I think an orchestration node makes sense.

turadg · 2024-06-13T13:55:43Z

packages/orchestration/src/exos/cosmosOrchestrationAccount.js

@@ -158,6 +158,7 @@ export const prepareCosmosOrchestrationAccountKit = (
      // must be the fully synchronous maker because the kit is held in durable state
      // @ts-expect-error XXX Patterns
      const topicKit = makeRecorderKit(storageNode, PUBLIC_TOPICS.account[1]);
+      void E(topicKit.recorder).write(harden({ sequence: 0n }));


It seems useful but it does have a cost. Is it just helpful for diagnostics or is there a product use case to publish each sequence?

If we do include it, consider a more transparent property name like executedSequence

turadg · 2024-06-13T13:58:52Z

packages/orchestration/src/exos/cosmosOrchestrationAccount.js

@@ -363,6 +366,7 @@ export const prepareCosmosOrchestrationAccountKit = (
         */
        async getBalance(denom) {
          const { chainAddress, icqConnection } = this.state;
+          if (!icqConnection) throw Error('Queries not enabled.');


This reads like something the developer could change.

Suggested change

if (!icqConnection) throw Error('Queries not enabled.');

if (!icqConnection) throw Fail'Queries not enabled for chain ${chainAddress.chainId}`.`;

Agree, updated

turadg · 2024-06-13T13:59:34Z

packages/orchestration/src/proposals/start-stakeOsmo.js

+  const storageNode = await makeStorageNodeChild(chainStorage, VSTORAGE_PATH);
+  const marshaller = await E(board).getPublishingMarshaller();
+
+  // TODO add osmo to bank


Came to realize the contract is not using the issuerKeywordRecord (depositing will be done via an LCA). Removed from here and start-stakeAtom.js

turadg · 2024-06-13T14:05:05Z

packages/orchestration/test/examples/stake-atom.contract.test.ts

-  //   t.regex(address.address, /agoric1/);
-  t.like(address, { chainId: 'cosmoshub-4', addressEncoding: 'bech32' });
+  const chainAddress = await E(account).getAddress();
+  // t.regex(address.address, /cosmos1/);


why disable?

It doesn't seem to be a meaningful test, since the value is currently UNPARSABLE_CHAIN_ADDRESS. Added a comment with FIXME mock remoteAddress in ibc bridge to here as well.

turadg · 2024-06-13T14:06:03Z

packages/orchestration/test/examples/stake-atom.contract.test.ts

+  t.log('inv', inv);
+
+  const seat = await E(zoe).offer(inv);
+  t.log('seat', seat);


logging values is useful when diagnosing a failure but noise when the tests are passing.

Acknowledged and removed. My interpretation of @dckc's stance on this topic leads me to be believe there are differing views, so trying to strike a balance.

turadg · 2024-06-13T14:07:37Z

packages/orchestration/test/examples/stake-atom.contract.test.ts

+
+  // FIXME mock remoteAddress in ibc bridge
+  const storagePath =
+    'mockChainStorageRoot.stakeAtom.accounts.UNPARSABLE_CHAIN_ADDRESS';


Not sure the solution but we definitely shouldn't write UNPARSABLE_CHAIN_ADDRESS to vstorage

turadg · 2024-06-13T14:08:48Z

packages/orchestration/test/staking-ops.test.ts

-  //   const { rootNode } = makeFakeStorageKit('mockChainStorageRoot');
-
-  const storageNode = Far('StorageNode', {}) as unknown as StorageNode;
+  const { rootNode } = makeFakeStorageKit('mockChainStorageRoot', {


consider a unique value for easier string search

Suggested change

const { rootNode } = makeFakeStorageKit('mockChainStorageRoot', {

const { rootNode } = makeFakeStorageKit('stakingOpsTest', {

turadg · 2024-06-13T14:09:16Z

packages/orchestration/test/staking-ops.test.ts

+  const { makeRecorderKit, storageNode, zcf, icqConnection, zone } = s;
+  const make = prepareCosmosOrchestrationAccountKit(zone, makeRecorderKit, zcf);
+
+  // Higher fidelity tests below use invitationMakers.


helpful comment

This is not actually the case 😅 . The main motivation for adding the test here was the TODO: when we write to chainStorage, test it. comment on L187.

I think this functionality is sufficiently tested elsewhere. There's a bit of overlap between staking-ops.test.ts and stake-atom.test.ts we might want to reduce at some point. My preference is to favor the former since it relies on fewer hand-rolled/localized mocks.

0xpatrickdev · 2024-06-13T19:24:37Z

packages/orchestration/src/exos/chainAccountKit.js

@@ -160,6 +160,8 @@ export const prepareChainAccountKit = zone =>
          this.state.remoteAddress = remoteAddr;
          this.state.localAddress = localAddr;
          this.state.chainAddress = harden({
+            // FIXME need a fallback value like icacontroller-1-connection-1 if this fails


Suggested change

// FIXME need a fallback value like icacontroller-1-connection-1 if this fails

// FIXME need a fallback value like icacontroller-1-connection-1 if this fails

// Or, we should throw since a consumer can't do much w/o an address.

turadg · 2024-06-15T19:09:57Z

packages/orchestration/src/cosmos-api.ts

@@ -151,7 +151,9 @@ export interface StakingAccountActions {
   * The unbonding time is padded by 10 minutes to account for clock skew.
   * @param delegations - the delegation to undelegate
   */
-  undelegate: (delegations: Delegation[]) => Promise<void>;
+  undelegate: (
+    delegations: Omit<Delegation, 'delegatorAddress'>[],


before release we should name these different types and define them additively (instead of subtracting)

but I think this is appropriate for now before we we've nailed down all the cases

- given the ChainInfo type is inferred from KnownChains, ensure the icqEnabled key is present on all cosmos chains to facilitate TS types

- a better approach seems to be refactoring start-stakeAtom.js to start-stakeIca.js with parameters. buildProposal doesn't seem to accept options so punted for now - TODO: register OSMO in bank

0xpatrickdev changed the title ~~feat(cosmosStakingAccount): write chain address to vstorage~~ feat(cosmosStakingAccount): publish chain address to vstorage Jun 11, 2024

0xpatrickdev commented Jun 11, 2024

View reviewed changes

0xpatrickdev marked this pull request as ready for review June 11, 2024 16:17

0xpatrickdev force-pushed the pc/9042-stake-atom-vstorage branch 3 times, most recently from ca6d46a to b313ac8 Compare June 12, 2024 21:42

0xpatrickdev requested a review from turadg June 12, 2024 23:24

turadg reviewed Jun 13, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/9042-stake-atom-vstorage branch from b313ac8 to bec424d Compare June 13, 2024 19:17

0xpatrickdev commented Jun 13, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/9042-stake-atom-vstorage branch 2 times, most recently from 4818f50 to 30bcdfa Compare June 14, 2024 20:09

turadg approved these changes Jun 15, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/9042-stake-atom-vstorage branch from 30bcdfa to f56928d Compare June 17, 2024 21:00

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Jun 17, 2024

0xpatrickdev added 6 commits June 17, 2024 21:15

fix(orchestration): makeAccount never resolves when icqEnabled: false

a74b6a2

feat(stakeAtom): publish address to vstorage

c2021ba

fix(chainInfo): ensure icqEnabled is present

b83745f

- given the ChainInfo type is inferred from KnownChains, ensure the icqEnabled key is present on all cosmos chains to facilitate TS types

feat(orchestration): add init-stakeOsmo.js to support .query tests

b6df6c2

- a better approach seems to be refactoring start-stakeAtom.js to start-stakeIca.js with parameters. buildProposal doesn't seem to accept options so punted for now - TODO: register OSMO in bank

fix(cosmosOrchestrationAccount): use AmountArg consistently

71403dd

fix(undelegate): delegatorAddress should not be user supplied

11aea65

0xpatrickdev force-pushed the pc/9042-stake-atom-vstorage branch from f56928d to 11aea65 Compare June 17, 2024 21:15

mergify bot merged commit 24665a9 into master Jun 17, 2024
77 checks passed

mergify bot deleted the pc/9042-stake-atom-vstorage branch June 17, 2024 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cosmosStakingAccount): publish chain address to vstorage #9482

feat(cosmosStakingAccount): publish chain address to vstorage #9482

0xpatrickdev commented Jun 11, 2024 •

edited

Loading

0xpatrickdev Jun 11, 2024

turadg Jun 13, 2024

0xpatrickdev Jun 13, 2024

0xpatrickdev Jun 11, 2024

turadg Jun 13, 2024

0xpatrickdev Jun 13, 2024

cloudflare-workers-and-pages bot commented Jun 11, 2024 •

edited

Loading

turadg left a comment

turadg Jun 13, 2024

turadg Jun 13, 2024

0xpatrickdev Jun 13, 2024

turadg Jun 13, 2024

turadg Jun 13, 2024

0xpatrickdev Jun 13, 2024

turadg Jun 13, 2024

0xpatrickdev Jun 13, 2024

turadg Jun 13, 2024

0xpatrickdev Jun 13, 2024 •

edited

Loading

turadg Jun 13, 2024

0xpatrickdev Jun 13, 2024

turadg Jun 13, 2024

turadg Jun 13, 2024

turadg Jun 13, 2024

0xpatrickdev Jun 13, 2024

0xpatrickdev Jun 13, 2024 •

edited

Loading

turadg Jun 15, 2024

		const { accountsStorageNode } = await provideAll(baggage, {
		accountsStorageNode: () => E(storageNode).makeChildNode('accounts'),

	if (!icqConnection) throw Error('Queries not enabled.');
	if (!icqConnection) throw Fail'Queries not enabled for chain ${chainAddress.chainId}`.`;

	const { rootNode } = makeFakeStorageKit('mockChainStorageRoot', {
	const { rootNode } = makeFakeStorageKit('stakingOpsTest', {

	// FIXME need a fallback value like icacontroller-1-connection-1 if this fails
	// FIXME need a fallback value like icacontroller-1-connection-1 if this fails
	// Or, we should throw since a consumer can't do much w/o an address.

feat(cosmosStakingAccount): publish chain address to vstorage #9482

feat(cosmosStakingAccount): publish chain address to vstorage #9482

Conversation

0xpatrickdev commented Jun 11, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

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

cloudflare-workers-and-pages bot commented Jun 11, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

turadg 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

0xpatrickdev Jun 13, 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

0xpatrickdev Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev commented Jun 11, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jun 11, 2024 •

edited

Loading

0xpatrickdev Jun 13, 2024 •

edited

Loading

0xpatrickdev Jun 13, 2024 •

edited

Loading