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

test updating instances in promiseSpace updates agoricNames #9989

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Chris-Hibbert
Copy link
Contributor

refs: #9970

Description

While preparing a coreEval for the vaults&Auctions release, I found it necessary to update both instance.produce.auctioneer and agoricNamesAdmin.lookupAdmin('instance').

This test shows that updating the former causes the latter to update.

Testing Considerations

@Chris-Hibbert Chris-Hibbert added test namehub-petname issuer naming, petnames, namehubs, ... labels Aug 28, 2024
@Chris-Hibbert Chris-Hibbert self-assigned this Aug 28, 2024
@Chris-Hibbert Chris-Hibbert marked this pull request as draft August 28, 2024 18:16
@Chris-Hibbert
Copy link
Contributor Author

For the purposes of conversation, this test shows that

 await space.instance.produce.auctioneer.reset();
  await space.instance.produce.auctioneer.resolve(auctionInstance2);

does indeed update agoricNames.

  const instanceLookup = await agoricNames.lookup('instance');
  t.is(await instanceLookup.lookup('auctioneer'), auctionInstance2);

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

This shows that the short version works in at least some cases.

It doesn't hurt to land this as-is, but should we look harder for problems first?

Comment on lines +75 to +81
const auctionInstance1 = makeHandle('instance1');
const instanceLookup = await agoricNames.lookup('instance');

// @ts-expect-error storing fake instance
space.instance.produce.auctioneer.resolve(auctionInstance1);
// @ts-expect-error expecting instance
t.is(await space.instance.consume.auctioneer, auctionInstance1);
Copy link
Member

Choose a reason for hiding this comment

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

I suggest moving the error to where auctionInstance1 is declared:

Suggested change
const auctionInstance1 = makeHandle('instance1');
const instanceLookup = await agoricNames.lookup('instance');
// @ts-expect-error storing fake instance
space.instance.produce.auctioneer.resolve(auctionInstance1);
// @ts-expect-error expecting instance
t.is(await space.instance.consume.auctioneer, auctionInstance1);
/** @type { Instance } */
// @ts-expect-error mock
const auctionInstance1 = makeHandle('instance1');
const instanceLookup = await agoricNames.lookup('instance');
space.instance.produce.auctioneer.resolve(auctionInstance1);
t.is(await space.instance.consume.auctioneer, auctionInstance1);

@Chris-Hibbert
Copy link
Contributor Author

It doesn't hurt to land this as-is, but should we look harder for problems first?

I'd like to look harder, but I don't know what to look for. My current (very tentative) hypothesis is that the promises are taking a while to resolve in the upgrade coreEval.

@dckc
Copy link
Member

dckc commented Aug 28, 2024

I'd like to look harder, but I don't know what to look for. My current (very tentative) hypothesis is that the promises are taking a while to resolve in the upgrade coreEval.

We can look there, I think, by controlling which promises resolve when in a test. We might have to introduce a few more moving parts. hm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
namehub-petname issuer naming, petnames, namehubs, ... test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants