fix(governance): some governance invitations use proposal patterns #7041

erights · 2023-02-21T20:48:32Z

Start replacing uses of the deprecated assertProposalShape with makeInvitations direct support for proposalShape patterns. This provides better protection for contracts against malformed offers. When an offer's proposal violates the contract's required proposalShape pattern, it is rejected in Zoe and the contract never even sees the malformed offer.

This PR is somewhat empirical, revising the provided proposal patterns so that they were tighter than the assertProposalPattern call it replaced, while still passing all tests. For each modified contract, it would be good for someone who knows the meaning of the contract to do a sanity check on the proposalShape pattern.

One particular way these patterns could be tighter, to protect the contract from seeing more malformed offers, is to use the amountShape specific to the brand the contract is willing to accept in that position. I skipped that additional work, leaving a TODO there instead. Reviewers, if you know what brand-specific amountShape I should use in any such case, please let me know.

Upgrade considerations

We definitely need to think hard about whether this PR introduces any representation changes that create an upgrade issue. The proposal shapes that we're less worried about were extracted into #8268 , leaving only some governance cases here.

dckc

I commented on the ones I know.

packages/governance/src/committee.js

packages/governance/src/electorateTools.js

packages/governance/src/noActionElectorate.js

packages/inter-protocol/src/stakeFactory/attestation.js

packages/inter-protocol/src/stakeFactory/stakeFactoryKit.js

dckc · 2023-02-28T06:18:01Z

packages/inter-protocol/src/stakeFactory/stakeFactoryKit.js

+ // Or should this be more like the AdjustBalancesProposalShape in
+ // vaultFactory, that allows both records to have both properties?


I don't think so... but only because I don't see a case where you would want to give both collateral and debt-brand, or want both. Why is that allowed in vaultFactory?

The requirement for VaultFactory AdjustBalances is that you can "give" or "want" without specifying the other.

I don't know either of a good use case for giving the same brands as you're wanting, but there's also not a strong reason to forbid that and allowing it makes the shape simpler to implement the above requirement.

Doesn't the StakeFactory have the same requirement? i.e. that you can specify one and the other is implied by your staked holdings. If so we lack test coverage for that case because this shape isn't throwing.

I can't tell whether this is redundant with what @turadg said, but I want to be explicit:

with VF AdjustBalances, you can give both or want both or give one and want the other. I don't think it ever makes sense to have the same keyword in both give and want.

If so we lack test coverage for that case ...

That's where my mind went too.

I'm not sure I can justify the time to swap in the relevant context for this just now.

The goal for this PR is to express with patterns at least the restrictions that were expressed by assertProposalShape and similar, but still be unrestrictive enough to allow everything we should be allowing. So, based on what I think I understood of the above comments, I did revise this pattern to be like the AdjustBalancesProposalShape from VF, that is more accepting.

PTAL and let me know if I misunderstood. Leaving this Conversation unresolved until I hear back.

packages/inter-protocol/src/stakeFactory/stakeFactoryKit.js

packages/pegasus/src/pegasus.js

packages/inter-protocol/src/vaultFactory/liquidateIncrementally.js

packages/inter-protocol/src/vaultFactory/vault.js

packages/inter-protocol/src/vaultFactory/vaultManager.js

packages/inter-protocol/src/vpool-xyk-amm/addLiquidity.js

packages/pegasus/src/pegasus.js

packages/zoe/src/contractSupport/zoeHelpers.js

packages/zoe/src/contracts/auction/index.js

packages/inter-protocol/src/stakeFactory/stakeFactoryKit.js

erights · 2023-02-28T21:18:45Z

packages/inter-protocol/src/stakeFactory/stakeFactoryKit.js

+ // Or should this be more like the AdjustBalancesProposalShape in
+ // vaultFactory, that allows both records to have both properties?


The goal for this PR is to express with patterns at least the restrictions that were expressed by assertProposalShape and similar, but still be unrestrictive enough to allow everything we should be allowing. So, based on what I think I understood of the above comments, I did revise this pattern to be like the AdjustBalancesProposalShape from VF, that is more accepting.

PTAL and let me know if I misunderstood. Leaving this Conversation unresolved until I hear back.

erights · 2023-02-28T21:26:05Z

packages/inter-protocol/src/stakeFactory/stakeFactory.js

+ zcf.makeInvitation(
+ seatx => helper.closeHook(seatx),
+ 'CloseVault',
+ undefined,
+ StakeFactoryCloseProposalShape,
+ ),


Seems annoyingly similar to the zcf.makeInvitation call in stakeFactoryKit function makeCloseInvitation. As a result, the proposalShape used here is annoyingly like the one used there. Can we consolidate both?

Leaving Conversation unresolved until I hear back.

seems like they could be unified. What's the obstacle? The factory has the branded amounts. Can the kit get them from there? Is there an obstacle to the Kit making the branded amounts itself?

@Chris-Hibbert I recall we discussed this, but I no longer remember where we landed. Do you?

I don't remember the conversation. Shall we try again?

packages/inter-protocol/src/vaultFactory/vault.js

packages/inter-protocol/src/vaultFactory/liquidateIncrementally.js

packages/inter-protocol/src/vpool-xyk-amm/removeLiquidity.js

datadog-full-agoric · 2023-03-04T00:14:51Z

Datadog Report

Branch report: markm-more-invitation-proposal-patterns
Commit report: 9fb3e17

✅ agoric-sdk: 0 Failed, 0 New Flaky, 3661 Passed, 57 Skipped, 19m 47.42s Wall Time

datadog-full-agoric · 2023-03-14T07:16:33Z

Datadog Report

Branch report: markm-more-invitation-proposal-patterns
Commit report: dbbd635

❌ agoric-sdk: 1 Failed (0 Known Flaky), 0 New Flaky, 2910 Passed, 29 Skipped, 17m 30.73s Wall Time

❌ Failed Tests (1)

bootstrapTests › demo-config › sim/demo config provides home with .myAddressNameAdmin - agoric.vats - Details

Expand for error

 ---
     name: AssertionError
     assertion: deepEqual
     values:
       'Difference (- actual, + expected):': |2-
           [
             'agoricNames',
         -   'bank',
         +   'attMaker',
         -   'behaviors',
 ...

erights · 2023-08-28T06:20:33Z

Reviewers,

Enough has changed that it is worth asking afresh: What's needed for me to move this forward? Because

this contributes to defensiveness against incoming messages,
may be easy since it has stayed green for a long time
makes more of our code a better example for others to follow

I would like to make progress on this. But no great urgency.

erights · 2023-08-28T06:29:44Z

If some cases remain problematic, I'd rather just postpone those to a later PR and make progress now on the non-problematic ones. Thanks.

dckc · 2023-08-28T14:54:38Z

What's needed for me to move this forward?

The emerging norm is to have upgrade tests for any contracts deployed in production; in particular, committee.js.

Pegasus is not (yet) in production, so landing that seems un-problematic. I think auction.js likewise.

For the helper functions, I'd like to see them traced back to any production bundles that they might appear in.

dckc

I'm not sure about landing changes to committee.js without at least a scheduled issue for upgrade testing. @turadg ?

turadg · 2023-08-28T19:16:58Z

I'm not sure about landing changes to committee.js without at least a scheduled issue for upgrade testing. @turadg ?

Given the low urgency of landing this and that we have upgrade-12 scheduled, I'd request such a test be included in this PR before considering landing it.

We definitely need to think hard about whether this PR introduces any representation changes that create an upgrade issue.

Please include this in an "Upgrade considerations" section in the main PR description.

erights · 2023-08-29T00:24:29Z

Please include this in an "Upgrade considerations" section in the main PR description.

Done

erights · 2023-08-29T00:27:21Z

I'm not sure about landing changes to committee.js without at least a scheduled issue for upgrade testing. @turadg ?

Given the low urgency of landing this and that we have upgrade-12 scheduled, I'd request such a test be included in this PR before considering landing it.

What if I postponed the committee.js changes to a later PR? What if I postponed all the packages/governance changes to a later PR?

dckc · 2023-08-29T02:06:17Z

Separating governance might do it. Would have to check whether the zoe changes affect any production bundles too.

erights · 2023-08-29T04:14:18Z

Zoe and Pegasus cases extracted to #8268 , leaving only the governance cases here.

Chris-Hibbert

Looks good.

erights self-assigned this Feb 21, 2023

erights force-pushed the markm-more-invitation-proposal-patterns branch 3 times, most recently from fce8418 to ef35a91 Compare February 28, 2023 03:34

erights requested review from dckc, turadg and Chris-Hibbert February 28, 2023 04:20

erights mentioned this pull request Feb 28, 2023

fix: Tighten invitation proposal patterns #6475

Closed

dckc reviewed Feb 28, 2023

View reviewed changes

turadg reviewed Feb 28, 2023

View reviewed changes

erights force-pushed the markm-more-invitation-proposal-patterns branch from fdcf70d to 933d539 Compare February 28, 2023 20:23

erights commented Feb 28, 2023

View reviewed changes

turadg reviewed Feb 28, 2023

View reviewed changes

packages/inter-protocol/src/vaultFactory/vault.js Outdated Show resolved Hide resolved

packages/inter-protocol/src/vaultFactory/liquidateIncrementally.js Outdated Show resolved Hide resolved

packages/inter-protocol/src/vpool-xyk-amm/removeLiquidity.js Outdated Show resolved Hide resolved

turadg mentioned this pull request Mar 1, 2023

feat(auction): add an auctioneer to manage vault liquidation #7000

Merged

erights force-pushed the markm-more-invitation-proposal-patterns branch from 995f366 to 05b90ff Compare March 1, 2023 01:46

turadg mentioned this pull request Mar 1, 2023

vaultFactory proposal shapes #7094

Merged

erights force-pushed the markm-more-invitation-proposal-patterns branch 6 times, most recently from 601f312 to 31551ac Compare March 3, 2023 23:56

erights requested a review from gibson042 March 4, 2023 20:51

erights force-pushed the markm-more-invitation-proposal-patterns branch 2 times, most recently from b0bd0e0 to 1b7cc36 Compare March 9, 2023 04:02

erights force-pushed the markm-more-invitation-proposal-patterns branch from 1b7cc36 to 7570893 Compare March 14, 2023 07:08

erights force-pushed the markm-more-invitation-proposal-patterns branch from 7570893 to 2b38651 Compare March 19, 2023 23:54

erights force-pushed the markm-more-invitation-proposal-patterns branch from 2b38651 to f56aa87 Compare April 19, 2023 04:19

erights force-pushed the markm-more-invitation-proposal-patterns branch from 342e10f to ae5ea19 Compare August 28, 2023 05:43

erights requested review from turadg and dckc August 28, 2023 06:16

dckc reviewed Aug 28, 2023

View reviewed changes

erights force-pushed the markm-more-invitation-proposal-patterns branch from ae5ea19 to 4cd5f10 Compare August 29, 2023 00:28

erights requested a review from dckc August 29, 2023 00:28

erights mentioned this pull request Aug 29, 2023

fix(zoe,pegasus): some zoe and pegasus invitations use proposal patterns #8268

Open

erights force-pushed the markm-more-invitation-proposal-patterns branch from 4cd5f10 to 9372909 Compare August 29, 2023 04:01

erights changed the base branch from master to markm-afew-invitation-proposal-patterns August 29, 2023 04:02

erights force-pushed the markm-more-invitation-proposal-patterns branch from 9372909 to b023677 Compare August 29, 2023 04:05

erights changed the base branch from markm-afew-invitation-proposal-patterns to master August 29, 2023 04:05

erights changed the title ~~fix: more invitations use proposal patterns~~ fix(governance): some governance invitations use proposal patterns Aug 29, 2023

erights force-pushed the markm-more-invitation-proposal-patterns branch from b023677 to 2df9334 Compare August 29, 2023 04:17

Chris-Hibbert approved these changes Aug 29, 2023

View reviewed changes

turadg approved these changes Aug 29, 2023

View reviewed changes

erights force-pushed the markm-more-invitation-proposal-patterns branch from 113fc49 to a620aaf Compare August 29, 2023 20:01

erights added the automerge:rebase Automatically rebase updates, then merge label Aug 29, 2023

erights added 2 commits August 29, 2023 22:51

fix(governance): some governance invitations use proposal patterns

0f794cb

fix: move EmptyProposalShape back here

1a0db7d

erights force-pushed the markm-more-invitation-proposal-patterns branch from a620aaf to 1a0db7d Compare August 29, 2023 22:51

mergify bot merged commit 1eed398 into master Aug 29, 2023
70 checks passed

mergify bot deleted the markm-more-invitation-proposal-patterns branch August 29, 2023 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(governance): some governance invitations use proposal patterns #7041

fix(governance): some governance invitations use proposal patterns #7041

erights commented Feb 21, 2023 •

edited

Loading

dckc left a comment

dckc Feb 28, 2023

turadg Feb 28, 2023 •

edited

Loading

Chris-Hibbert Feb 28, 2023

dckc Feb 28, 2023

erights Feb 28, 2023

erights Feb 28, 2023

erights Feb 28, 2023

Chris-Hibbert Mar 1, 2023

erights Mar 14, 2023

Chris-Hibbert Mar 14, 2023

datadog-full-agoric bot commented Mar 4, 2023 •

edited

Loading

datadog-full-agoric bot commented Mar 14, 2023 •

edited

Loading

erights commented Aug 28, 2023

erights commented Aug 28, 2023

dckc commented Aug 28, 2023

dckc left a comment

turadg commented Aug 28, 2023

erights commented Aug 29, 2023

erights commented Aug 29, 2023

dckc commented Aug 29, 2023

erights commented Aug 29, 2023

Chris-Hibbert left a comment

		// Or should this be more like the AdjustBalancesProposalShape in
		// vaultFactory, that allows both records to have both properties?

fix(governance): some governance invitations use proposal patterns #7041

fix(governance): some governance invitations use proposal patterns #7041

Conversation

erights commented Feb 21, 2023 • edited Loading

Upgrade considerations

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datadog-full-agoric bot commented Mar 4, 2023 • edited Loading

Datadog Report

datadog-full-agoric bot commented Mar 14, 2023 • edited Loading

Datadog Report

❌ Failed Tests (1)

erights commented Aug 28, 2023

erights commented Aug 28, 2023

dckc commented Aug 28, 2023

dckc left a comment

Choose a reason for hiding this comment

turadg commented Aug 28, 2023

erights commented Aug 29, 2023

erights commented Aug 29, 2023

dckc commented Aug 29, 2023

erights commented Aug 29, 2023

Chris-Hibbert left a comment

Choose a reason for hiding this comment

erights commented Feb 21, 2023 •

edited

Loading

turadg Feb 28, 2023 •

edited

Loading

datadog-full-agoric bot commented Mar 4, 2023 •

edited

Loading

datadog-full-agoric bot commented Mar 14, 2023 •

edited

Loading