test: depositing to and withdrawing from a remote ica account #10211

0xpatrickdev · 2024-10-03T21:14:16Z

closes: #9193

Description

adds e2e tests for Deposit and Withdraw invitationMakers for LocalOrchestrationAccount via the basic-flows contract
adds e2e test demonstrating depositing to and withdrawing from an ICA in a PortfolioHolder via the basic-flows contract
updates PortfolioHolder's Proxying invitationMaker to optionally accept invitationArgs

Security Considerations

n/a, primarily tests

Scaling Considerations

n/a, primarily tests

Documentation Considerations

These tests serve as documentation for using the Deposit, Withdraw, Transfer, and Proxying invitationMakers from the Orchestration API

Testing Considerations

PR includes E2E tests to close out the referenced issue.

Upgrade Considerations

Library code for NPM Orch release

dckc · 2024-10-03T21:26:01Z

scope question...

closes: #9193

#9193 depends on #9784, which is open. How can this close #9193 without closing #9784?

0xpatrickdev · 2024-10-03T21:38:05Z

scope question...

closes: #9193

#9193 depends on #9784, which is open. How can this close #9193 without closing #9784?

I think that's a mistake. I am going to remove it from the requirements. I also adjusted the description for #9784 to better reflect the current state of things. .transfer() / Transfer is implemented, just not to the same level as it is in LocalOrchAccount. Getting notified of a successful transfer is not a current capability and needs design.

dckc

sharing my comments so far

dckc · 2024-10-03T22:05:11Z

multichain-testing/test/deposit-withdraw-lca.test.ts

+  const { deleteTestKeys, setupTestKeys, ...rest } = await commonSetup(t);
+  deleteTestKeys(accounts).catch();
+  const wallets = await setupTestKeys(accounts);


Aside...

In Agoric/dapp-orchestration-basics#61 where I folded in your work on starship testing, I struggled a bit to get this commonSetup(t) stuff working. In order to be able to test it without re-deploying my contract for each iteration (or worse: standing up the whole cluster again), I factored out the ambient authority so I could inject mocks for quick ava testing.

See test('orca-multichain.test style usage', ...).
https://github.com/Agoric/dapp-orchestration-basics/blob/63f85352eaa73659d4cadbc1f17d48dd4095d98c/contract/test/agd-lib.test.js#L234

and test('deploy-cli style usage', ...)
https://github.com/Agoric/dapp-orchestration-basics/blob/main/contract/test/agd-lib.test.js#L214

Here's hoping we can migrate that back to support.js here in due course.

Nice, that's a good approach for this problem. We added startContact in 3639831 a bit ago that ensures the contract is only installed once by querying vstorage

I wonder what the best path is for keeping testing tools in sync with minimal effort. Releases were something I'd floated in the past but it didn't seem to get much support.

dckc · 2024-10-03T22:09:50Z

multichain-testing/test/deposit-withdraw-lca.test.ts

+    invitationSpec: {
+      source: 'agoricContract',
+      instancePath: [contractName],
+      callPipe: [['makeOrchAccountInvitation']],


devex note: it would be nice if our contracts came with a supplementary module of constants so that client didn't have to come up with magic strings like this.

For example: https://github.com/Agoric/agoric-sdk/blob/master/packages/inter-protocol/src/clientSupport.js

+1

client support module for Orchestration #10214

turadg

Awesome to have this user story tested e2e

I'd like to improve the abstractions in the tests so they're more reviewable by Product, but that can wait

turadg · 2024-10-03T23:11:44Z

multichain-testing/test/deposit-withdraw-lca.test.ts

+
+  const accountStoragePath = Object.fromEntries(offerToPublicSubscriberPaths)[
+    makeAccountOfferId
+  ]?.account;


I'm surprised this optional doesn't cause a problem for the .split on 73. Maybe offerToPublicSubscriberPaths is `any? Not a blocker but consider typing.

Suggested change

]?.account;

]!.account;

Yes, offerToPublicSubscriberPaths is any. Typing that here doesn't feel right - I've added a TODO for #10214

turadg · 2024-10-03T23:14:29Z

multichain-testing/test/deposit-withdraw-portfolio.test.ts

+
+    // IBC Transfer funds to remoteChain account
+    const remoteChainId = remoteChainInfo.chain.chain_id;
+    const ibcTransferOfferId = `Transfer-to-${chainName}-${Date.now()}`;


the other offer ids are lowercase

…ionMaker

- creates a `PortfolioHolder` account that's comprised of a local and remote orchAccounts - uses `Deposit` and `Transfer` invitationMakers (via `Proxying`) to send funds to `remoteOrchAccount` - uses `Transfer` and `Withdraw` invitationMakers (via `Proxying`) to retrieve funds from `remoteOrchAccount`

cloudflare-workers-and-pages · 2024-10-04T15:49:35Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`ba81770`
Status:	✅ Deploy successful!
Preview URL:	https://269bfedc.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-deposit-withdraw-e2e.agoric-sdk.pages.dev

View logs

0xpatrickdev

@0xpatrickdev in Documentation Considerations of #10211, you write...

These tests serve as documentation for using the Deposit, Withdraw, Transfer, and Proxying invitationMakers from the Orchestration API

Do we have any JSDoc for those? I'm struggling to find PortfolioHolderKit at all in @agoric/orchestration API docs.

Good question @dckc . I exported the types for PortfolioHolder and MakeCombineInvitationMakers in the recent commits and improved the copy.

See: https://e46fc33c.agoric-sdk.pages.dev/funcs/_agoric_orchestration.preparePortfolioHolder and https://e46fc33c.agoric-sdk.pages.dev/funcs/_agoric_orchestration.prepareCombineInvitationMakers

Edit: moved to its own PR: #10217

0xpatrickdev · 2024-10-04T15:48:38Z

multichain-testing/test/deposit-withdraw-lca.test.ts

+
+  const accountStoragePath = Object.fromEntries(offerToPublicSubscriberPaths)[
+    makeAccountOfferId
+  ]?.account;


Yes, offerToPublicSubscriberPaths is any. Typing that here doesn't feel right - I've added a TODO for #10214

0xpatrickdev added the force:integration Force integration tests to run on PR label Oct 3, 2024

0xpatrickdev marked this pull request as ready for review October 3, 2024 21:23

0xpatrickdev requested a review from a team as a code owner October 3, 2024 21:23

0xpatrickdev requested review from AgoricTriage, turadg and dckc October 3, 2024 21:23

dckc mentioned this pull request Oct 3, 2024

Incomplete jsdocs for Orchestration API types, variables, methods and parameters #9757

Open

dckc reviewed Oct 3, 2024

View reviewed changes

turadg mentioned this pull request Oct 3, 2024

client support module for Orchestration #10214

Open

turadg approved these changes Oct 3, 2024

View reviewed changes

0xpatrickdev added 3 commits October 4, 2024 11:47

test: deposit to and withdraw from local-orch-account

4e129e2

feat(portfolio-holder): optional invitationArgs in Proxying invitat…

47e754e

…ionMaker

0xpatrickdev force-pushed the pc/deposit-withdraw-e2e branch from 6021289 to 4a2108a Compare October 4, 2024 15:49

0xpatrickdev force-pushed the pc/deposit-withdraw-e2e branch 3 times, most recently from 9d82dfa to 53f76a5 Compare October 4, 2024 16:26

0xpatrickdev commented Oct 4, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/deposit-withdraw-e2e branch from 53f76a5 to ba81770 Compare October 4, 2024 17:09

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

mergify bot mentioned this pull request Oct 4, 2024

orchestration typedoc: portfolio holder #10217

Merged

mergify bot merged commit 097ef82 into master Oct 4, 2024
87 of 95 checks passed

mergify bot deleted the pc/deposit-withdraw-e2e branch October 4, 2024 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: depositing to and withdrawing from a remote ica account #10211

test: depositing to and withdrawing from a remote ica account #10211

0xpatrickdev commented Oct 3, 2024 •

edited

Loading

dckc commented Oct 3, 2024

0xpatrickdev commented Oct 3, 2024

dckc left a comment

dckc Oct 3, 2024

0xpatrickdev Oct 3, 2024

dckc Oct 3, 2024

turadg Oct 3, 2024 •

edited

Loading

turadg left a comment

turadg Oct 3, 2024

0xpatrickdev Oct 4, 2024

turadg Oct 3, 2024

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

edited

Loading

0xpatrickdev left a comment •

edited

Loading

0xpatrickdev Oct 4, 2024

test: depositing to and withdrawing from a remote ica account #10211

test: depositing to and withdrawing from a remote ica account #10211

Conversation

0xpatrickdev commented Oct 3, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

dckc commented Oct 3, 2024

0xpatrickdev commented Oct 3, 2024

dckc left a comment

Choose a reason for hiding this comment

dckc Oct 3, 2024

Choose a reason for hiding this comment

0xpatrickdev Oct 3, 2024

Choose a reason for hiding this comment

dckc Oct 3, 2024

Choose a reason for hiding this comment

turadg Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

turadg Oct 3, 2024

Choose a reason for hiding this comment

0xpatrickdev Oct 4, 2024

Choose a reason for hiding this comment

turadg Oct 3, 2024

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Oct 4, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

0xpatrickdev left a comment • edited Loading

Choose a reason for hiding this comment

0xpatrickdev Oct 4, 2024

Choose a reason for hiding this comment

0xpatrickdev commented Oct 3, 2024 •

edited

Loading

turadg Oct 3, 2024 •

edited

Loading

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

edited

Loading

0xpatrickdev left a comment •

edited

Loading