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

Fix/offer handler test not resolving vow from async-flow #25

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Jovonni
Copy link
Collaborator

@Jovonni Jovonni commented Aug 16, 2024

Fixes #20 , good catch @dckc 🚀

@Jovonni Jovonni self-assigned this Aug 16, 2024
@Jovonni Jovonni requested a review from dckc August 16, 2024 22:14
@Jovonni
Copy link
Collaborator Author

Jovonni commented Aug 16, 2024

CI fails from:

➤ YN0028: +
➤ YN0028: -    "@agoric/zone": "npm:^0.2.2"
➤ YN0028: +    "@agoric/zone": "npm:dev"
➤ YN0028: The lockfile would have been modified by this install, which is explicitly forbidden.
➤ YN0000: └ Completed
➤ YN0000: · Failed with errors in 27s 944ms

@agoric/zone package version was updated due to makeHeapVow not being exported on the previous version used.

How do we allow this change here?

@Jovonni Jovonni marked this pull request as ready for review August 16, 2024 22:18
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.

Changes LGTM. Before merging please squash the fixup commit. (E.g. git rebase --interactive --autosquash)

Note that #20's title is "contract test to act like UI client". This fixes the one problem described in the issue body but I'm not certain it satisfies the title well enough to close that issue. You know this app much better than I so I leave that to you to decide.

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.

There's a 3rd getOfferResult call that should also get the vt.when(...) treatment.

@@ -201,6 +230,55 @@ const makeTestContext = async t => {
},
});

const localchain = Far('DummyLocalchain', {
getChainHub: async () => {
Copy link
Member

Choose a reason for hiding this comment

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

localchain doesn't have a getChainHub method, does it?

[
'ubld',
{
brand: '$0.Alleged: BLD brand',
Copy link
Member

Choose a reason for hiding this comment

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

The value here should be a brand, not a string, right?

Why use a custom namehub implementation? boot-tools.js sets up agoricNames and agoricNamesAdmin in the boostratp promise space.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

seeing this when I try to using agoricNames from boot-tools as you mention here:

orca-contract › orchestrate - osmosis makeAccount and fund returns a ContinuingOfferResult
  Rejected promise returned by test. Reason:

  Error {
    message: 'chain not found:agoric',
  }

  › makeError (file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/ses/src/error/assert.js:349:61)
  › eval (.../async-flow/src/convert.js:100:1)
  › innerConvert (.../async-flow/src/convert.js:57:8)
  › convertRecur (.../async-flow/src/convert.js:26:8)
  › Alleged: replayMembrane.convert [as guestToHost] (.../async-flow/src/convert.js:76:1)
  › eval (.../async-flow/src/async-flow.js:265:37)

and

Error#1: "nameKey" not found: "chain"
    at makeError (file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/ses/src/error/assert.js:349:61)
    at Function.fail (file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/ses/src/error/assert.js:481:20)
    at NonNullish (file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/@agoric/internal/src/errors.js:14:10)
    at Object.lookup (file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/@agoric/vats/src/nameHub.js:177:15)
    at In "lookup" method of (NameHubKit nameHub) [as lookup] (file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/@endo/exo/src/exo-tools.js:171:24)
    at localApplyMethod (file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/@endo/eventual-send/src/local.js:126:18)
    at Object.applyMethod (file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/@endo/eventual-send/src/handled-promise.js:455:16)
    at dispatchToHandler (file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/@endo/eventual-send/src/handled-promise.js:156:22)
    at doDispatch (file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/@endo/eventual-send/src/handled-promise.js:486:7)
    at win (file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/@endo/eventual-send/src/handled-promise.js:505:26)
    at file:///Users/jovonni/Documents/projects/dapp-orchestration-basics/node_modules/@endo/eventual-send/src/handled-promise.js:523:20

digging

contract/test/orca-contract.test.js Outdated Show resolved Hide resolved
contract/test/orca-contract.test.js Outdated Show resolved Hide resolved
contract/test/orca-contract.test.js Show resolved Hide resolved
@turadg turadg self-requested a review August 19, 2024 20:30
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.

Will review when CI is green

…mocks for localchain.makeAccount, and vBankAssets. Prettier applied as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

contract test to act like UI client
3 participants