fix(orchestration): remove `cancelToken` + rename `createAccount` -> `makeAccount` #9312

0xpatrickdev · 2024-05-01T17:25:29Z

closes: #XXXX
refs: #9207

Description

removes cancelToken logic from .undelegate()
renames createAccount to makeAccount for orchestration and localchain (Method Naming Structure)
- make<Foo>(): Creates a new Foo object and returns only that object.
- create<Foo>(): Creates a new Foo, but doesn't return it.
renames getChainAddress -> getAddress on orchestration

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages · 2024-05-01T17:28:41Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`39a980a`
Status:	✅ Deploy successful!
Preview URL:	https://cb3db7ec.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-fix-orchestration-types.agoric-sdk.pages.dev

View logs

Chris-Hibbert

Thanks. This seems like the right renaming.

Chris-Hibbert · 2024-05-01T17:38:37Z

packages/boot/test/bootstrapTests/test-orchestration.ts

@@ -140,7 +140,7 @@ test.serial('stakeAtom - smart wallet', async t => {
    invitationSpec: {
      source: 'agoricContract',
      instancePath: ['stakeAtom'],
-      callPipe: [['makeCreateAccountInvitation']],
+      callPipe: [['makeAccountInvitation']],


If you hate makeMakeAccountInvitation, how about makeNewAccountInvitation?

I'm okey if you don't want to change this.

yeah, makeAccountInvitation seems misleading

oh is it a function to make the maker? I did not parse that. makeMakeAccountInvitation?

Dismissing since initial scope has increased

dckc

this looks consistent with recent feedback from @dtribble

dckc · 2024-05-01T20:18:38Z

packages/orchestration/src/types.d.ts

+  Brand,
+  Payment,
+  Purse,
+  RemotableBrand,


RemotableBrand comes from @endo/eventual-send, right? Importing it from ERTP is sort of misleading, suggesting it has something to do with the ERTP Brand type.

RemotableBrand doesn't seem to be used. why add it?

Artifact, removed. Thanks for surfacing

dckc · 2024-05-01T20:22:45Z

packages/boot/test/bootstrapTests/test-orchestration.ts

@@ -140,7 +140,7 @@ test.serial('stakeAtom - smart wallet', async t => {
    invitationSpec: {
      source: 'agoricContract',
      instancePath: ['stakeAtom'],
-      callPipe: [['makeCreateAccountInvitation']],
+      callPipe: [['makeAccountInvitation']],


yeah, makeAccountInvitation seems misleading

dtribble

The comments are minor and straightforward. Consider it reviewed once you've addressed them. Making it "request changes" so it doesn't autosubmit....

dtribble · 2024-05-01T21:17:14Z

packages/boot/test/bootstrapTests/test-orchestration.ts

@@ -140,7 +140,7 @@ test.serial('stakeAtom - smart wallet', async t => {
    invitationSpec: {
      source: 'agoricContract',
      instancePath: ['stakeAtom'],
-      callPipe: [['makeCreateAccountInvitation']],
+      callPipe: [['makeNewAccountInvitation']],


should be makeAccountInvitation?

This was discussed above - it's a function that makes an invitation to make an account, makeMakeAccountInvitation .

I don't feel strongly either way - makeAccountInvitation is potentially misleading, makeNewAccountInvitation could be too since we don't use "new" anywhere here.

I think the historical pattern for this would be makeAcountInvitationMaker, which then would produce something you would call makeAccountInvitation

dtribble · 2024-05-01T21:17:37Z

packages/boot/test/bootstrapTests/test-vat-orchestration.ts

  const {
    runUtils: { EV },
  } = t.context;

  const orchestration = await EV.vat('bootstrap').consumeItem('orchestration');

-  const account = await EV(orchestration).createAccount(
+  const account = await EV(orchestration).makeAccount(


Why does this need EV?

EV is a testing utility for bootstrap tests

It's like E but works through RunUtils.

agoric-sdk/packages/SwingSet/tools/run-utils.js

Lines 55 to 61 in 231416e

// IMPORTANT WARNING TO USERS OF `EV`

//

// `EV` presents an abstraction that can be used (within tests only!) to get

// much of the convenience with respect to messaging that `E` provides in

// normal code. However, this convenience comes with a huge caveat that all

// users of this convenience feature MUST keep in mind.

//

dtribble · 2024-05-01T21:19:54Z

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

      hostConnectionId,
      controllerConnectionId,
    );
-    const accountAddress = await E(account).getAccountAddress();
-    trace('account address', accountAddress);
+    const address = await E(account).getAddress();


btw feel free to name the variable accountAddress if that seems more informative

~~👍 going to keep it as address for consistency~~. I might lobby to change getRemoteAddress() to getRemoteChannelAddress() (and getLocalAddress() -> getLocalChannelAddress()) so no one gets confused by these.

On second thought, since I am going to change things to use ChainAddress, i will call the local variable chainAddress. Was awkward typing this.state.address.address to form a message.

dtribble · 2024-05-01T21:20:44Z

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

      },
-      makeCreateAccountInvitation() {
+      makeNewAccountInvitation() {


-makeAccountInvitation? (make implies new)-
Discussion above; this proposed name misses the usage. makeAcountInvitationMaker would follow historical patterns for this, but it's not critical.

dtribble · 2024-05-01T21:23:10Z

packages/orchestration/src/exos/stakingAccountKit.js

- *  account: ChainAccount;
- *  chainAddress: string;
+ *  account: ChainAccountKit['account'];
+ *  address: ChainAddress['address'];


Why are we not including the chainID as well? If we have staking reports that are Eth, it should clear from the log?

I would love to have a standard simple format that encompasses all addresses, but we just aren't seeing it yet.

We have #9162 for work around the ChainAddress object. I will stub that out now in this PR so we can remove the ChainAccountKit['account'] references.

dtribble · 2024-05-01T21:24:06Z

packages/orchestration/src/service.js

@@ -59,7 +59,7 @@ export const Proto3Shape = {
 };

 export const ChainAccountI = M.interface('ChainAccount', {
-  getAccountAddress: M.call().returns(M.string()),
+  getAddress: M.call().returns(M.string()),


This should also return a ChainAddress.

otherwise it cannot be passed as an arg to the various "send" operations

dtribble · 2024-05-01T21:25:06Z

packages/orchestration/src/service.js

       * }}
       */ (
        harden({
          port,
          connection: undefined,
          requestedRemoteAddress,
          remoteAddress: undefined,
-          accountAddress: undefined,
+          address: undefined,


should this be "localAddress" in comparison to remote address? or is it "sourceAddress"?

localAddress means something different - it's the channel address on the local chain. Here is console output from a test:

ICA Account Addresses { accountAddress: 'cosmos1test', localAddress: '/ibc-port/icacontroller-1/ordered/{"version":"ics27-1","controllerConnectionId":"connection-0","hostConnectionId":"connection-0","address":"cosmos1test","encoding":"proto3","txType":"sdk_multi_msg"}/ibc-channel/channel-1', remoteAddress: '/ibc-hop/connection-0/ibc-port/icahost/ordered/{"version":"ics27-1","controllerConnectionId":"connection-0","hostConnectionId":"connection-0","address":"cosmos1test","encoding":"proto3","txType":"sdk_multi_msg"}/ibc-channel/channel-NaN', }

We need to extract address from the negotiated remote channel address.

dtribble · 2024-05-01T21:26:50Z

packages/orchestration/src/service.js

@@ -252,9 +252,9 @@ const prepareOrchestration = (zone, createChainAccount) =>
         *   the counterparty connection_id
         * @param {IBCConnectionID} controllerConnectionId
         *   self connection_id
-         * @returns {Promise<ChainAccount>}
+         * @returns {Promise<ChainAccountKit['account']>}


This kind of type seems like a source of confusion rather than help. They well defined type that it should return is a ChainAccount?

Added a6360a5 to remove the confusion

dtribble · 2024-05-01T21:27:57Z

packages/orchestration/src/types.d.ts

@@ -430,7 +421,7 @@ export type TransferMsg = {

 // Example
 // await icaNoble.transferSteps(usdcAmt,
-//   osmosisSwap(tiaBrand, { pool: 1224, slippage: 0.05 }, icaCel.getAddress()));
+//   osmosisSwap(tiaBrand, { pool: 1224, slippage: 0.05 }, icaCel.getChainAddress()));


Shouldn't this be getAddress?

dckc · 2024-05-02T15:16:07Z

packages/orchestration/src/exos/chainAccountKit.js

+          this.state.localAddress = localAddr;
+          // XXX parseAddress currently throws, should it return '' instead?
+          this.state.chainAddress = harden({
+            address: parseAddress(remoteAddr) || UNPARSABLE_CHAIN_ADDRESS,


The name parseAddress suggests that it's taking something like agoric123lj23kj2... and picking it apart. I had to read the source code -- well, the tests, actually -- to figure out what it's doing. How about changing its argument type to RemoteIbcAddress? and maybe rename it to findAddressField?

p.s. I sure wish we used tools that make it easier to re-use tests as documentation a la doctest.

Good naming suggestion. Will tackle it here if there's add'l feedback, or tackle it as part of #9198.

Also planning to rename makeICAConnectionAddress to makeICAChannelAddress

dckc

seems like forward progress

turadg · 2024-05-02T16:19:18Z

packages/orchestration/src/types.d.ts

   */
-  undelegate: (delegations: Delegation[]) => Promise<Undelegation[]>;
+  undelegate: (delegations: Delegation[]) => Promise<void>;


why not return at least MsgUndelegateResponse?

I don't think cosmjs types like that show up anywhere else in OrchestrationAccount, do they?

I was going to say that MsgUndelegateResponse is empty anyway, but it's not; it has vital info: completion_time.

I think this makes sense. Should we also include MsgUndelegateResponse, MsgBeginRedelegateResponse, and MsgTransferResponse? MsgDelegateResponse and MsgSend are empty objects

Consider it reviewed once you've addressed them. Making it "request changes" so it doesn't autosubmit....

- to follow [Method Naming Structure](https://docs.agoric.com/guides/ertp/#method-naming-structure)

0xpatrickdev requested review from turadg and Chris-Hibbert May 1, 2024 17:25

Chris-Hibbert previously approved these changes May 1, 2024

View reviewed changes

0xpatrickdev marked this pull request as draft May 1, 2024 18:18

0xpatrickdev force-pushed the pc/fix-orchestration-types branch 2 times, most recently from a3e4c25 to cad3dcd Compare May 1, 2024 20:11

0xpatrickdev requested a review from dtribble May 1, 2024 20:16

0xpatrickdev marked this pull request as ready for review May 1, 2024 20:16

dckc approved these changes May 1, 2024

View reviewed changes

dtribble previously requested changes May 1, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/fix-orchestration-types branch 2 times, most recently from 2de7c5a to a6360a5 Compare May 2, 2024 13:44

0xpatrickdev requested review from dckc and dtribble May 2, 2024 13:45

dckc reviewed May 2, 2024

View reviewed changes

dckc approved these changes May 2, 2024

View reviewed changes

turadg reviewed May 2, 2024

View reviewed changes

turadg approved these changes May 2, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/fix-orchestration-types branch from ca624fd to 182dd0f Compare May 2, 2024 17:53

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label May 2, 2024

0xpatrickdev added 6 commits May 2, 2024 14:16

fix(orchestration): remove cancelToken logic from undelegate

be14fa5

refactor(orchestration): rename createAccount -> makeAccount

330ebea

- to follow [Method Naming Structure](https://docs.agoric.com/guides/ertp/#method-naming-structure)

fix(orchestration): rename .getAccountAddress -> .getAddress

f951cde

refactor(localchain): createAccount -> makeAccount

05cce52

refactor(orchestration): prefer ChainAddress object to string

f2a6d41

feat(orchestration): align ChainAccount spec with current implementation

678f21f

0xpatrickdev force-pushed the pc/fix-orchestration-types branch from 182dd0f to a3aeff0 Compare May 2, 2024 18:16

0xpatrickdev added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels May 2, 2024

refactor(chainAccountKit): move to own file

39a980a

0xpatrickdev force-pushed the pc/fix-orchestration-types branch from a3aeff0 to 39a980a Compare May 2, 2024 20:10

mergify bot merged commit 5a4779a into master May 2, 2024
64 checks passed

mergify bot deleted the pc/fix-orchestration-types branch May 2, 2024 20:34

This was referenced May 2, 2024

feat(orchestration): icq balance query #9198

Merged

types for chain addresses #9162

Closed

	// IMPORTANT WARNING TO USERS OF `EV`
	//
	// `EV` presents an abstraction that can be used (within tests only!) to get
	// much of the convenience with respect to messaging that `E` provides in
	// normal code. However, this convenience comes with a huge caveat that all
	// users of this convenience feature MUST keep in mind.
	//

fix(orchestration): remove cancelToken + rename createAccount -> makeAccount #9312

fix(orchestration): remove cancelToken + rename createAccount -> makeAccount #9312

Conversation

0xpatrickdev commented May 1, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented May 1, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtribble May 2, 2024 • edited Loading

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

dtribble 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

0xpatrickdev May 1, 2024 • edited Loading

Choose a reason for hiding this comment

0xpatrickdev May 1, 2024 • edited Loading

Choose a reason for hiding this comment

dtribble May 1, 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 May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev May 1, 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

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

fix(orchestration): remove `cancelToken` + rename `createAccount` -> `makeAccount` #9312

fix(orchestration): remove `cancelToken` + rename `createAccount` -> `makeAccount` #9312

0xpatrickdev commented May 1, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented May 1, 2024 •

edited

Loading

dtribble May 2, 2024 •

edited

Loading

0xpatrickdev May 1, 2024 •

edited

Loading

0xpatrickdev May 1, 2024 •

edited

Loading

dtribble May 1, 2024 •

edited

Loading

0xpatrickdev May 1, 2024 •

edited

Loading

0xpatrickdev May 1, 2024 •

edited

Loading