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

8606 invite proposals #10719

Merged
merged 1 commit into from
Jan 7, 2025
Merged

8606 invite proposals #10719

merged 1 commit into from
Jan 7, 2025

Conversation

Chris-Hibbert
Copy link
Contributor

closes: #8606
closes: #8597

Description

#8597 suggested adding a test proposal guards in Zoe. This adds that test, and also cleans up a redundant check in coveredCall. The added test exposed a mistake in the Zoe doc, so 1258 fixes that as well.

Security Considerations

None

Scaling Considerations

None.

Documentation Considerations

Improved documentation.

Testing Considerations

Added tests.

Upgrade Considerations

These changes do not impact code on-chain. There's a correction to an example contract, an improvement in a TypeGuard, and a documentation update which will be separately release.

@Chris-Hibbert Chris-Hibbert added Zoe package: Zoe test Zoe Contract Contracts within Zoe labels Dec 17, 2024
@Chris-Hibbert Chris-Hibbert self-assigned this Dec 17, 2024
@Chris-Hibbert Chris-Hibbert requested a review from a team as a code owner December 17, 2024 21:26
Copy link

cloudflare-workers-and-pages bot commented Dec 17, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6cd9b9a
Status: ✅  Deploy successful!
Preview URL: https://1432d27a.agoric-sdk.pages.dev
Branch Preview URL: https://8606-invite-proposals.agoric-sdk.pages.dev

View logs

@@ -367,7 +367,7 @@ export const ZoeServiceI = M.interface('ZoeService', {
}),
getInvitationDetails: M.call(M.eref(InvitationShape)).returns(M.any()),
getProposalShapeForInvitation: M.call(InvitationHandleShape).returns(
M.opt(ProposalShape),
M.opt(M.pattern()),
Copy link
Member

Choose a reason for hiding this comment

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

why does getProposalShapeForInvitation return values other than a ProposalShape?

Is ProposalShape wrong?

I notice that getProposalShapeForInvitation typedef returns Pattern | undefined but some places then require that it be narrower than Pattern:

const proposalShape =
offerDataAccess.getProposalShapeForInvitation(invitationHandle);
if (proposalShape !== undefined) {
mustMatch(proposal, proposalShape, `${q(description)} proposal`);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Different meta-levels: proposalShape is the type for something that is shaped like a proposal. E(zoe).offer(invitation, proposal) takes a proposal, and its guard requires that it match proposalShape.

  const proposal = harden({
    give: { B: moola(5n) },
    exit: { onDemand: null },
  });
  E(zoe).offer(invitation, proposal, { B: fiveMoola });

The object returned by getProposalShapeForInvitation(invitationHandle) describes a proposal. It's a pattern.

  const proposalShape = M.splitRecord({
    give: { B: AmountShape },
    exit: { deadline: M.any() },
  });
  const invitation = await zcf.makeInvitation(
    boring,
    'seat1',
    {},
    proposalShape,
  );
  const { handle } = await E(zoe).getInvitationDetails(invitation);
  const shape = await E(zoe).getProposalShapeForInvitation(handle);
  t.deepEqual(shape, proposalShape);

It's arguable whether getProposalShapeForInvitation should return a ProposalShapePattern, but I don't think it's possible to customize M.pattern().

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for explaining. I agree Pattern is the right type but I request that getProposalShapeForInvitation get a jsdoc explaining its return type. A good time also to inline GetProposalShapeForInvitation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Chris-Hibbert Chris-Hibbert force-pushed the 8606-invite-proposals branch from f75a179 to f7b444c Compare January 6, 2025 23:45
@Chris-Hibbert Chris-Hibbert added automerge:squash Automatically squash merge and removed automerge:squash Automatically squash merge labels Jan 7, 2025
Copy link
Contributor

mergify bot commented Jan 7, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #10719 has been dequeued. The pull request rule doesn't match anymore. The following conditions don't match anymore:

  • label=automerge:squash
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue squash]
      • any of:
        • check-pending=integration-test-result
        • check-success=integration-test-result
        • label=bypass:integration

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@Chris-Hibbert Chris-Hibbert force-pushed the 8606-invite-proposals branch from cfb3a62 to bff77ff Compare January 7, 2025 00:59
@Chris-Hibbert Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Jan 7, 2025
correct one typeGuard

improve jsdoc

Also noticed that the types documented for
getProposalShapeForInvitation were incorrect. fixed that with
Agoric/documentation#1258
@Chris-Hibbert Chris-Hibbert force-pushed the 8606-invite-proposals branch from bff77ff to 6cd9b9a Compare January 7, 2025 01:47
@mergify mergify bot merged commit 9c97014 into master Jan 7, 2025
81 checks passed
@mergify mergify bot deleted the 8606-invite-proposals branch January 7, 2025 02:40
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 test Zoe Contract Contracts within Zoe Zoe package: Zoe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zoe should have a test of proposalShape.
2 participants