refactor!: new reallocate API to assist with reviewing conservation of rights #3184

katelynsills · 2021-05-26T01:39:39Z

This PR makes a breaking change to ZCFSeats, zoe.reallocate, and the trade, swap, and swapExact helpers.

SeatStagings and zcfSeat.stage no longer exist. Instead, amountKeywordRecords can be added or subtracted to a seat's allocation by calling methods directly on the zcfSeat:

{(amountKeywordRecord: AmountKeywordRecord) => AmountKeywordRecord} incrementBy - the amountKeywordRecord to be added to the seat's staged allocation
{(amountKeywordRecord: AmountKeywordRecord) => AmountKeywordRecord} decrementBy - the amountKeywordRecord to be subtracted from the seat's staged allocation
{() => void} clear - delete the staged allocation, if any
{() => Allocation} getStagedAllocation - get the stagedAllocation, the allocation that will be committed if the seat is reallocated over, if offer safety holds and rights are conserved
{() => boolean} hasStagedAllocation - whether there is a staged allocation, i.e. whether incrementBy or decrementBy has been called and clear has not.

Once a seat has a staged allocation, it can be reallocated over by calling zcf.reallocate(zcfSeat1, zcfSeat2, ....). This commits the staged allocation, such it is now the current allocation for that seat.

The trade helper no longer exists, and the swap and swapExact helpers no longer accept errorMsg arguments.

Example usage:

      seat.decrementBy({ In: reducedAmountIn });
      poolSeat.incrementBy({ Central: reducedAmountIn });

      poolSeat.decrementBy({ Secondary: amountOut });
      seat.incrementBy({ Out: amountOut });

      zcf.reallocate(poolSeat, seat);
      seat.exit();

Closes #3215

#dapp-otc-branch: 3184-reallocate-api

erights

Review so far. Not finished yet.

I am very encouraged by the overall line shrinkage of this PR!

packages/pegasus/src/pegasus.js

packages/treasury/src/collectRewardFees.js

packages/zoe/src/contractFacet/allocationMath.js

packages/zoe/src/contractFacet/types.js

erights · 2021-06-01T01:49:06Z

packages/treasury/src/vault.js

+    if (proposal.want.Collateral) {
+      vaultSeat.decrementBy(
+        seat.incrementBy({ Collateral: proposal.want.Collateral }),
+      );
+    } else if (proposal.give.Collateral) {


Is it possible that a proposal has both a want.Collateral and a give.Collateral?

I think it is possible that a proposal has both, and from I can tell, the code prior to this PR does not prevent it or account for it.

packages/treasury/src/vault.js

packages/zoe/src/contractFacet/contractFacet.js

packages/zoe/src/contracts/autoswap.js

packages/zoe/src/contracts/coveredCall.js

katelynsills · 2021-06-05T01:00:45Z

@erights, this is now ready for review!

katelynsills · 2021-06-05T01:02:04Z

@Chris-Hibbert, could you review the changes to newSwap and packages/treasury? I changed newSwap such that the transfer of rights is easy to audit for conservation.

Chris-Hibbert

I think it would be better for transferReward() to return feeSeat, so there is a single reallocate() call.

The refactoring in swap.js looks clean to me. Thanks!

I would drop the new TODOs in swap.js, but I won't insist on it if my reasoning isn't persuasive.

packages/treasury/src/stablecoinMachine.js

packages/treasury/src/vault.js

packages/zoe/src/contracts/newSwap/swap.js

erights

LGTM

All suggested changes are about error messages, comments, and one renaming.

packages/zoe/src/contractFacet/allocationMath.js

packages/zoe/src/contractSupport/types.js

erights · 2021-06-08T01:55:05Z

packages/zoe/src/contractFacet/zcfSeat.js

+   * @returns {void}
+   */
+  const commitStagedAllocation = zcfSeat => {
+    assertActive(zcfSeat);


This assert happens after the commit point. If it fails it throws. If it throws, it might throw after some allocations have been committed. This is an excellent example of an assert that should immediately shut down at lease the contract. We already have a zcf.assert for this purpose. However, this is not the assert severity you want for normal uses of assertActive, so you'd end up with an annoying duplication of code.

Any check failure that might fail must happen before the commit point. If the failure here is already obviously impossible, or once it is, then a comment here is fine. Impossible cases can become possible under maintenance mistakes. If the impossible happens and zcfSeat is not active then you're guaranteed a low level failure on line 61 below. When I put in atomic actions, I will need to ensure that any such "cannot happen" throws cause vat termination anyway.

Good point. I moved this check earlier and removed it from this function, as I don't think it was being adequately checked before.

packages/zoe/src/contractFacet/zcfSeat.js

erights · 2021-06-08T02:17:57Z

packages/zoe/src/contractFacet/zcfSeat.js

+  const commitStagedAllocation = zcfSeat => {
+    assertActive(zcfSeat);
+    activeZCFSeats.set(zcfSeat, zcfSeat.getStagedAllocation());
+    zcfSeatToStagedAllocations.delete(zcfSeat);


The delete on line 62 seems exactly the same as the set on line 61. It should be impossible to get here if zcfSeat is not in zcfSeatToStagedAllocations. If that is not already obviously impossible, it must be made obviously impossible. If it happens anyway we reliable get a low level failure from the delete, which throws within the atomic action. Once I use the atomic action API to ensure that such failures cause immediate vat failure, we're safe even against these specific cannot-happen conditions.

When I will ask you to review the PR that protects such atomic actions, please keep these "impossible" cases in mind.

That sounds good. I'll add a comment about this.

erights · 2021-06-08T02:35:21Z

packages/zoe/src/contractFacet/zcfSeat.js

 import { E } from '@agoric/eventual-send';
 import { AmountMath } from '@agoric/ertp';
 import { Far } from '@agoric/marshal';

 import { isOfferSafe } from './offerSafety';
 import { assertRightsConserved } from './rightsConservation';
+import { addToAllocation, subtractFromAllocation } from './allocationMath';

 /** @type {CreateSeatManager} */
 export const createSeatManager = (zoeInstanceAdmin, getAssetKindByBrand) => {


There is no type SeatManager or SeatKit. But from the comment on CreateSeatManager I would guess that the record returned at the bottom is the SeatManager. If so, then per our previous discussion, I do not understand why this is createSeatManager rather than makeSeatManager.

This is unchanged in this PR, so I won't address it here, but I think this is fine, since seatManager is an abstract concept only used internally. Since there is no seatManager reified as an object, I think create is fine as well.

Chris-Hibbert

One missed duplicated edit, one question. Looking good.

packages/treasury/test/vault-contract-wrapper.js

Chris-Hibbert · 2021-06-08T16:27:44Z

packages/zoe/src/contracts/newSwap/swap.js

+    // TODO: this should be transferred directly from the user such
+    // that there's no way to accidentally take more than expected
+    // from the pool.


Sorry, I didn't mark this copy of the TODO. I think it should also be dropped.

Ah, I left it in purposefully. In this case, it is central to secondary, so we can take the protocolFee directly from the user's give

…ng the `zcfSeat` and `reallocate` API

katelynsills added the Zoe package: Zoe label May 26, 2021

katelynsills self-assigned this May 26, 2021

katelynsills force-pushed the 3064-new-reallocate-api branch 3 times, most recently from 3dbb551 to fce73b9 Compare May 29, 2021 00:48

tyg mentioned this pull request May 30, 2021

Document new Reallocate API (update existing docs) Agoric/documentation#511

Closed

erights mentioned this pull request Jun 1, 2021

Suggestion for 3184 -- decrement first #3224

Merged

erights requested changes Jun 1, 2021

View reviewed changes

erights self-requested a review June 2, 2021 03:26

erights mentioned this pull request Jun 4, 2021

Category error: RUN is a coin, not a role #3252

Closed

katelynsills force-pushed the 3064-new-reallocate-api branch 2 times, most recently from 6bd2a98 to cc83838 Compare June 5, 2021 00:04

katelynsills requested a review from Chris-Hibbert June 5, 2021 01:00

katelynsills mentioned this pull request Jun 7, 2021

chore: use the new reallocate API rather than trade Agoric/dapp-otc#18

Merged

Chris-Hibbert requested changes Jun 7, 2021

View reviewed changes

erights approved these changes Jun 8, 2021

View reviewed changes

Chris-Hibbert approved these changes Jun 8, 2021

View reviewed changes

katelynsills force-pushed the 3064-new-reallocate-api branch from 7c1d537 to 5165993 Compare June 8, 2021 20:29

katelynsills mentioned this pull request Jun 8, 2021

Reallocate api patch Agoric/documentation#519

Merged

katelynsills added 4 commits June 8, 2021 13:56

refactor(zoe)!: make it easier to audit conservation of rights changi…

fe9f96e

…ng the `zcfSeat` and `reallocate` API

chore: bump CI

b192eb5

chore: address PR comments

12d5165

chore: address PR comments

a60e41b

katelynsills force-pushed the 3064-new-reallocate-api branch from 5165993 to a60e41b Compare June 8, 2021 20:56

katelynsills enabled auto-merge (squash) June 8, 2021 20:56

katelynsills merged commit f34e5ea into master Jun 8, 2021

katelynsills deleted the 3064-new-reallocate-api branch June 8, 2021 21:22

katelynsills mentioned this pull request Jun 17, 2021

refactor!(zoe): handle subtracting empty in a less fragile way #3345

Merged

katelynsills mentioned this pull request Sep 18, 2021

Design flaw in Zoe: stateful seats and staged allocations #3850

Closed

Chris-Hibbert mentioned this pull request Dec 4, 2023

test(coveredCall): Add a case for rejection of bad proposals #8597

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: new reallocate API to assist with reviewing conservation of rights #3184

refactor!: new reallocate API to assist with reviewing conservation of rights #3184

katelynsills commented May 26, 2021 •

edited

erights left a comment

erights Jun 1, 2021

katelynsills Jun 4, 2021

katelynsills commented Jun 5, 2021

katelynsills commented Jun 5, 2021

Chris-Hibbert left a comment

erights left a comment

erights Jun 8, 2021

katelynsills Jun 8, 2021

erights Jun 8, 2021

katelynsills Jun 8, 2021

erights Jun 8, 2021

katelynsills Jun 8, 2021

Chris-Hibbert left a comment

Chris-Hibbert Jun 8, 2021

katelynsills Jun 8, 2021

refactor!: new reallocate API to assist with reviewing conservation of rights #3184

refactor!: new reallocate API to assist with reviewing conservation of rights #3184

Conversation

katelynsills commented May 26, 2021 • edited

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katelynsills commented Jun 5, 2021

katelynsills commented Jun 5, 2021

Chris-Hibbert left a comment

Choose a reason for hiding this comment

erights left a 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

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katelynsills commented May 26, 2021 •

edited