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: generic chain with sendAnywhere demo contract #9460

Merged
merged 10 commits into from
Jun 10, 2024
Merged

Conversation

dckc
Copy link
Member

@dckc dckc commented Jun 5, 2024

closes #9187

Description

DRAFT until

  • what to do about the way transfer() maps chainId to transferChannel?
  • clean up git history
  • update other facade callers
  • integrate more fully with well-known chains: hot-new-chain gets added and "Nothing breaks."
  • what to do with facade contract upgrade test, now that chainInfos is a caller thing?

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

@dckc dckc requested review from turadg and 0xpatrickdev June 5, 2024 23:49
Copy link

cloudflare-workers-and-pages bot commented Jun 5, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8d90251
Status: ✅  Deploy successful!
Preview URL: https://c4c42129.agoric-sdk.pages.dev
Branch Preview URL: https://dc-send-anywhere.agoric-sdk.pages.dev

View logs

@dckc dckc force-pushed the dc-send-anywhere branch 2 times, most recently from f7e2e2a to ab38074 Compare June 6, 2024 20:28
@dckc dckc changed the base branch from master to 9063-agoricInfo June 6, 2024 20:29
Base automatically changed from 9063-agoricInfo to master June 6, 2024 21:27
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

nothing blocking; we'll keep iterating

Comment on lines +241 to +242
const agoricChainInfo = await chainHub.getChainInfo('agoric');
const { transferChannel } = await chainHub.getConnectionInfo(
Copy link
Member

Choose a reason for hiding this comment

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

given that these each do remote calls to the same vat, we should have a convinience for getting both

Copy link
Member Author

Choose a reason for hiding this comment

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

thought about it, but it's necessarily 2 round-trips: 1 to get the chainInfo, then you have to pick out the chainId and send it for the 2nd call

return {
/** @returns {Promise<ChainInfo>} */
async getChainInfo() {
return mockLocalChainInfo;
},

async makeAccount() {
const account = await E(localchain).makeAccount();
const lowLevelAccountP = E(localchain).makeAccount();
Copy link
Member

Choose a reason for hiding this comment

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

naming by "level" is a smell.

makeLocalChainAccountKit is making something that does OrchestrationAccount and more. Let's rename it to LocalOrchestrationAccount like the RemoteOrchestrationAccount.

Up to you whether this PR. I'm happy to do it. If you want to land this as-is please at least leave a TODO comment.

@@ -94,6 +99,8 @@ const makeLocalChainFacade = localchain => {
};

/**
* XXX should be provide...Facade
Copy link
Member

Choose a reason for hiding this comment

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

alternately keep this but also export provideOrchestrationFacade that ensures it runs at most once per zone.

Copy link
Member Author

Choose a reason for hiding this comment

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

provide isn't straightforward: maker return value {"orchestrate":"[Function orchestrate]"} is not storable

Copy link
Member

Choose a reason for hiding this comment

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

the stored parts would just be the kinds. The facade need not be memoized since it's stateless.

packages/orchestration/src/utils/chainHub.js Outdated Show resolved Hide resolved
Comment on lines 19 to 20
export const makeChainHub = agoricNames => {
// XXX not reachably-durable? really?? take a zone arg?
Copy link
Member

Choose a reason for hiding this comment

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

Seems reasonable and cheap to allow the caller to choose

Suggested change
export const makeChainHub = agoricNames => {
// XXX not reachably-durable? really?? take a zone arg?
export const makeChainHub = (agoricNames, zone = makeHeapZone()) => {

we can iterate on what the best way to use it is. it might differ from contract to contract.


const info = await chain.getChainInfo();
const { chainId } = info;
await E(contractAccount).deposit(await pmtP, amt);
Copy link
Member

Choose a reason for hiding this comment

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

should theis be pipelined?

I really don't know what's allowed within an async flow

(and you point out, contractAccount is not remote itself)

async (orch, { zcf }, seat, offerArgs) => {
mustMatch(
offerArgs,
harden({ chainKey: M.scalar(), destAddr: M.string() }),
Copy link
Member

Choose a reason for hiding this comment

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

consider a more explicit name for chainKey to distinguish from chainName and chainId (both keys)

Copy link
Member Author

Choose a reason for hiding this comment

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

going with chainName in anticipation of supporting well-known chains too

@dckc dckc force-pushed the dc-send-anywhere branch 3 times, most recently from 55aea8d to 995a831 Compare June 7, 2024 22:01
const [currentTime, timerBrand] = await Promise.all([
E(timer).getCurrentTimestamp(),
brandCache || E(timer).getTimerBrand(),
]);
const timeout =
Copy link
Member Author

Choose a reason for hiding this comment

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

@0xpatrickdev I managed to avoid passing timerBrand to every orchestration contract.

@@ -42,6 +48,7 @@ test('chain info', async t => {
t.deepEqual(await result.getChainInfo(), mockChainInfo);
});

// XXX what to do with this test now that chainInfos is the caller's responsibility?
Copy link
Member Author

Choose a reason for hiding this comment

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

@turadg what to do with this test now that chainInfos is the caller's responsibility?

Copy link
Member

Choose a reason for hiding this comment

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

hmm yeah, the main point of it was the name registry behavior..

My first thought it so leave it around as a stub for other "contract upgrade" assertions, but if someone shows up to the test as-is they're going to be confused why it exists. So I think it best be removed.

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 pruned it to a test.todo()

@dckc dckc force-pushed the dc-send-anywhere branch 2 times, most recently from 2f9963d to 0431291 Compare June 8, 2024 01:08
@dckc dckc requested a review from turadg June 8, 2024 01:09
@dckc dckc marked this pull request as ready for review June 10, 2024 14:05
@dckc
Copy link
Member Author

dckc commented Jun 10, 2024

@turadg updating the other callers to makeOrchestrationFacade (2165de7) is beyond what I think we discussed when you approved this. Take another look, please?

@dckc dckc changed the title feat: generic chain with sendAnywhere demo contract (WIP) feat: generic chain with sendAnywhere demo contract Jun 10, 2024

await Promise.all(registrationPromises);
for await (const [name, info] of Object.entries(wellKnownChainInfo)) {
log(`registering chain ${name}`);
Copy link
Member

Choose a reason for hiding this comment

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

in #9477 I found it helpful to be a little more specific since this log output doesn't have any context qualifier:

Suggested change
log(`registering chain ${name}`);
log(`registering agoricName chain.${name}`);

Though now that there's a registerChain method the logging could go there.

@@ -94,6 +99,8 @@ const makeLocalChainFacade = localchain => {
};

/**
* XXX should be provide...Facade
Copy link
Member

Choose a reason for hiding this comment

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

the stored parts would just be the kinds. The facade need not be memoized since it's stateless.

import { wellKnownChainInfo } from '../../src/chain-info.js';

const agoricChainInfo = wellKnownChainInfo.agoric;
import { commonSetup } from '../supports.js';
Copy link
Member

Choose a reason for hiding this comment

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

lint

@@ -38,8 +33,11 @@ export const commonSetup = async t => {
// To test durability in unit tests, test a particular entity with `relaxDurabilityRules: false`.
// To test durability integrating multiple vats, use a RunUtils/bootstrap test.
const rootZone = makeHeapZone();
const bld = withAmountUtils(makeIssuerKit('BLD'));
const ist = withAmountUtils(makeIssuerKit('IST'));
const { mint: _b, ...bld } = withAmountUtils(makeIssuerKit('BLD'));
Copy link
Member

Choose a reason for hiding this comment

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

this could use some explaining. a perhaps more legible approach would be an option to withAmountUtils

Copy link
Member Author

Choose a reason for hiding this comment

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

The concern seems specific to pourPayment and makeFakeBankManagerKit so explaining here seems like the way to go.

Copy link
Member

Choose a reason for hiding this comment

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

Fair. If we have to do this a lot we might want an AmountUtils for runutils tests which takes pourPayment and abstracts it.

Something to consider in the course of designing the API for writing tests that work in multiple levels of integration. (Unit, swingset, chain)

@dckc dckc added the automerge:rebase Automatically rebase updates, then merge label Jun 10, 2024
@mergify mergify bot merged commit 96c19f5 into master Jun 10, 2024
63 checks passed
@mergify mergify bot deleted the dc-send-anywhere branch June 10, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Orchestration - Create Generic Chain (Chain Object for non well known chain)
2 participants