feat: implement `.transfer` on `CosmosOrchestrationAccount` #9882

0xpatrickdev · 2024-08-12T23:26:14Z

Description

These changes primarily implement the .transfer method on CosmosOrchestrationAccount. The changes require providing the exo with a ChainHub for looking up transferChannel info and TimerService for building a default timeout parameter. Notably, the .transfer() Promise resolves when the host chain submits the transfer to other remote chain. The sendThenWaitForAck logic that LocalOrchestrationAccount has is not yet implemented but tracked via #9784.

Other changes include:

simplified transferChannel lookup logic in LocalOrchestrationAccount
/boot/tools/support.ts changes to simplify sendPacket ack mocks for dibc bridge
bootstrap tests for LocalOrchAccount.transfer() using Transfer invitationMaker
/boot/tools/support.ts changes to ensure mock localChain addresses are unique
packetUtils interface guard change for an observed, but seemingly benign, error

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

how to best document the differences between LOA and COA for the interim?

Testing Considerations

Includes unit and boot tests.

Upgrade Considerations

n/a, unreleased code

0xpatrickdev · 2024-08-12T23:27:10Z

packages/boot/tools/ibc/mocks.js

@@ -1,4 +1,5 @@
 // @ts-check
+import { createMockAckMap } from '@agoric/orchestration/tools/ibc-mocks.ts';


.ts doesn't seem right here. Is the correct way to address this a build step in the orchestration package.json?

Just change it to .js. TS will resolve the module.

Suggested change

import { createMockAckMap } from '@agoric/orchestration/tools/ibc-mocks.ts';

import { createMockAckMap } from '@agoric/orchestration/tools/ibc-mocks.js';

I don't recall why boot has allowImportingTsExtensions: true. I'll see about disabling it to avoid this problem.

a build step in the orchestration package.json?

Aside from cosmic-proto which uses code gen, we avoid build steps for dependencies between packages in the repo. @agoric/orchestration does transpile in its NPM publishing.

After rebasing on #9884 and changing to .js I see 🤔 :

$ ava test/orchestration/restart-contracts.test.ts Uncaught exception in test/orchestration/restart-contracts.test.ts Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/0xpatrick/repos/agoric-sdk/node_modules/@agoric/orchestration/tools/ibc-mocks.js' imported from /Users/0xpatrick/repos/agoric-sdk/packages/boot/tools/ibc/mocks.js › h (file:///Users/0xpatrick/repos/agoric-sdk/node_modules/@esbuild-kit/esm-loader/dist/index.js:12:1164)

pushed a fix in c4d45bf

Thanks. Still seeing https://github.com/Agoric/agoric-sdk/actions/runs/10419531180/job/28857889091?pr=9882#step:5:115. The problem goes away if we rename mocks.js and mocks.test.js to .ts, but I'll hold that fixup as I suspect a different solution might be preferred.

shoot, I tested locally but missed that one I guess.

more .ts seems like the best solution to me

this branch is behind master now. when you make that change feel free to rebase on master and force push with fixups squashed

0xpatrickdev · 2024-08-12T23:28:47Z

packages/boot/tools/supports.ts

            }
+            // An error that would be triggered before reception on another chain
+            return ackImmediately(obj, protoMsgMocks.error.ack);


// An error that would be triggered before reception on another chain

Thinking on this more, I suspect the relayer could also be delayed. Any harm in making this ackLater?

Seems like a trade-off. IIRC:

a sendPacket message could have an immediate failure before leaving the local node (i.e. just over the bridge) or after a reply from a relayer.

we can't reliably distinguish between those cases here

If (2) is false, then let's distinguish.

If (2) is true then we have to pick the better way to handle both. The pro of ackLater is that it let's us test doing other work before the ack arrives. The con is that it's something someone could forget in a test, and even when they don't it's more work.

The other immediate con is that it may break some tests and need fixing.

I don't have a strong stance on which is better. Whatever infidelity you choose, please leave a comment explaining the decision.

Thinking on this more, I also don't have a strong stance. It seems the main benefit of ackLater is to allow us to control timing in the test context. ackImmediately seems like a sensible default, as someone can always choose to supply a mock to the bridge that will use ackLater.

Nonetheless, sharing some thoughts/findings to your question -

a sendPacket message could have an immediate failure before leaving the local node (i.e. just over the bridge) or after a reply from a relayer.

Correct, the sendPacket handler can fail via ReceiveSendPacket before leaving agoric golang and going out to a relayer.

It seems this might only fail if an invalid sourcePort and sourceChannel are provided. I'm not sure when ibc.js or network.js would allow this since .downcall() is heavily guarded with ocaps + closures - the only thing coming to mind is bad state where a channel is closed but ibc/network are unaware.

cloudflare-workers-and-pages · 2024-08-12T23:45:31Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`e3ce160`
Status:	✅ Deploy successful!
Preview URL:	https://394bd972.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-cosmos-orch-transfer.agoric-sdk.pages.dev

View logs

turadg

Good stuff! Thanks for the clean commits for reviewing. Happy to see the code improvements along the way.

packages/orchestration/src/utils/time.js

packages/orchestration/src/exos/local-orchestration-account.js

packages/orchestration/index.js

packages/builders/scripts/testing/restart-stakeAtom.js

packages/orchestration/src/exos/cosmos-orchestration-account.js

turadg · 2024-08-13T13:37:01Z

packages/orchestration/src/exos/cosmos-orchestration-account.js

+                ? 0n
+                : E(timestampHelper).getTimeoutTimestampNS());
+
+            // Resolves when host chain successfully submits, but not when


the commit message made me think the feature was implemented. I see it just says it's adding the method, and I suppose there will be another feat commit that it works as expected.

turadg · 2024-08-13T13:37:50Z

packages/orchestration/src/exos/cosmos-orchestration-account.js

-  makeRecorderKit,
-  vowTools,
-  zcf,
+  { chainHub, makeRecorderKit, timerService, vowTools, zcf },


good refactor. I suppose this should be the habit from the outset. powers tend to expand

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

_incidental_ ## Description A couple packages had `allowImportingTsExtensions: true`. It recently caused [some confusion](#9882 (comment)). The reason we had it is so tests written in `.js` could import `.ts` modules. (Without this `.ts` import, the Ava transpiler config wouldn't try to resolve `.js` as a `.ts` module in a `.js` file.) But most of our tests in `boot` are `.ts` now and I think it would be best for them all to be. This removes that allowance and brings the imports into conformity. It also converts the one test that was using `.ts` in `.js` to be a `.ts` test. ### Security Considerations n/a ### Scaling Considerations n/a ### Documentation Considerations n/a ### Testing Considerations CI ### Upgrade Considerations n/a

- remove an unnecessary vat roundtrip

- to address: Error: Target 'agoric1mockVlocalchainAddress' already registered

must be .ts for .ts modules to find it by its .js module name

0xpatrickdev commented Aug 12, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/cosmos-orch-transfer branch from f8370b2 to 1ba74b7 Compare August 12, 2024 23:45

0xpatrickdev force-pushed the pc/cosmos-orch-transfer branch from 1ba74b7 to d5cb94b Compare August 12, 2024 23:46

0xpatrickdev requested a review from turadg August 13, 2024 00:29

0xpatrickdev marked this pull request as ready for review August 13, 2024 00:30

turadg mentioned this pull request Aug 13, 2024

import .ts modules with .js #9884

Merged

turadg reviewed Aug 13, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/cosmos-orch-transfer branch from d5cb94b to b79a25d Compare August 15, 2024 22:35

turadg approved these changes Aug 16, 2024

View reviewed changes

0xpatrickdev and others added 14 commits August 16, 2024 10:45

feat: get timerBrand from currentTime TimestampRecord

ab95b23

- remove an unnecessary vat roundtrip

feat: simplify transferChannel lookup

ee8b806

chore: ibc/applications/transfer/v1/tx.js in package exports

4d57f81

feat: parseOutgoingTxPacket test util

cbe876b

feat: guard orchAccount.transfer opts

02cb704

feat: export createMockAckMap helper

ed5998f

refactor: simplify sendPacket switch statement

6a90c4c

refactor: use SIMULATED_ERRORS const

8f4fe48

feat: add transfer method to CosmosOrchestrationAccount

b1fdde1

feat: ensure mock lca address is unique

1e5f500

- to address: Error: Target 'agoric1mockVlocalchainAddress' already registered

feat: LocalOrchestrationAccount Transfer invitation

b88db1e

types: opts: IBCMsgTransferOptions are optional

3481ec5

refactor: simplify stakeIca proposals

9567d1c

refactor: rename ibc/mocks.ts

e3ce160

must be .ts for .ts modules to find it by its .js module name

0xpatrickdev force-pushed the pc/cosmos-orch-transfer branch from c4d45bf to e3ce160 Compare August 16, 2024 14:45

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Aug 16, 2024

mergify bot merged commit bef1be9 into master Aug 16, 2024
90 checks passed

mergify bot deleted the pc/cosmos-orch-transfer branch August 16, 2024 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement `.transfer` on `CosmosOrchestrationAccount` #9882

feat: implement `.transfer` on `CosmosOrchestrationAccount` #9882

0xpatrickdev commented Aug 12, 2024

0xpatrickdev Aug 12, 2024

turadg Aug 13, 2024

0xpatrickdev Aug 15, 2024

turadg Aug 16, 2024

0xpatrickdev Aug 16, 2024

turadg Aug 16, 2024

turadg Aug 16, 2024

0xpatrickdev Aug 12, 2024

turadg Aug 13, 2024 •

edited

Loading

0xpatrickdev Aug 16, 2024

cloudflare-workers-and-pages bot commented Aug 12, 2024 •

edited

Loading

turadg left a comment

turadg Aug 13, 2024

turadg Aug 13, 2024

		@@ -1,4 +1,5 @@
		// @ts-check
		import { createMockAckMap } from '@agoric/orchestration/tools/ibc-mocks.ts';

feat: implement .transfer on CosmosOrchestrationAccount #9882

feat: implement .transfer on CosmosOrchestrationAccount #9882

Conversation

0xpatrickdev commented Aug 12, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Aug 12, 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

feat: implement `.transfer` on `CosmosOrchestrationAccount` #9882

feat: implement `.transfer` on `CosmosOrchestrationAccount` #9882

turadg Aug 13, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Aug 12, 2024 •

edited

Loading