From c626f59c7918f132bd370a3af388a752a92fc232 Mon Sep 17 00:00:00 2001 From: Kate Sills Date: Mon, 20 Sep 2021 16:50:58 -0700 Subject: [PATCH 1/5] fix: fix bug which breaks rights conservation and offer safety guarantees --- packages/zoe/src/contractFacet/zcfSeat.js | 82 ++++--- packages/zoe/src/contractFacet/zcfZygote.js | 70 ++++-- packages/zoe/src/internal-types.js | 7 +- .../zcf/test-reallocateForZCFMint.js | 211 ++++++++++++++++++ packages/zoe/test/unitTests/zcf/test-zcf.js | 4 +- 5 files changed, 317 insertions(+), 57 deletions(-) create mode 100644 packages/zoe/test/unitTests/zcf/test-reallocateForZCFMint.js diff --git a/packages/zoe/src/contractFacet/zcfSeat.js b/packages/zoe/src/contractFacet/zcfSeat.js index 8709723a111..62027414b80 100644 --- a/packages/zoe/src/contractFacet/zcfSeat.js +++ b/packages/zoe/src/contractFacet/zcfSeat.js @@ -113,34 +113,10 @@ export const createSeatManager = ( * and so can be used internally for reallocations that violate * conservation. * - * @type {ReallocateInternal} + * @type {reallocateForZCFMint} */ - const reallocateInternal = seats => { - // This check was already done in `reallocate`, but - // zcfMint.mintGains and zcfMint.burnLosses call - // `reallocateInternal` directly without calling `reallocate`, so - // we must check here as well. - seats.forEach(assertActive); - seats.forEach(assertStagedAllocation); - - // Keep track of seats used so far in this call, to prevent aliasing. - const zcfSeatsSoFar = new Set(); - - seats.forEach(seat => { - assert( - zcfSeatToSeatHandle.has(seat), - X`The seat ${seat} was not recognized`, - ); - assert( - !zcfSeatsSoFar.has(seat), - X`Seat (${seat}) was already an argument to reallocate`, - ); - zcfSeatsSoFar.add(seat); - }); - + const reallocateForZCFMint = (zcfSeat, newAllocation) => { try { - // No side effects above. All conditions checked which could have - // caused us to reject this reallocation. // COMMIT POINT // All the effects below must succeed "atomically". Scare quotes because // the eventual send at the bottom is part of this "atomicity" even @@ -148,15 +124,14 @@ export const createSeatManager = ( // updates from zcf to zoe, its effects must occur immediately in zoe // on reception, and must not fail. // - // Commit the staged allocations (currentAllocation is replaced - // for each of the seats) and inform Zoe of the + // Commit the newAllocation and inform Zoe of the // newAllocation. - seats.forEach(commitStagedAllocation); + activeZCFSeats.set(zcfSeat, newAllocation); - const seatHandleAllocations = seats.map(seat => { + const seatHandleAllocations = [zcfSeat].map(seat => { const seatHandle = zcfSeatToSeatHandle.get(seat); - return { seatHandle, allocation: seat.getCurrentAllocation() }; + return { seatHandle, allocation: newAllocation }; }); E(zoeInstanceAdmin).replaceAllocations(seatHandleAllocations); @@ -208,8 +183,47 @@ export const createSeatManager = ( X`At least one seat has a staged allocation but was not included in the call to reallocate`, ); - // Note COMMIT POINT within reallocateInternal - reallocateInternal(seats); + // Keep track of seats used so far in this call, to prevent aliasing. + const zcfSeatsSoFar = new Set(); + + seats.forEach(seat => { + assert( + zcfSeatToSeatHandle.has(seat), + X`The seat ${seat} was not recognized`, + ); + assert( + !zcfSeatsSoFar.has(seat), + X`Seat (${seat}) was already an argument to reallocate`, + ); + zcfSeatsSoFar.add(seat); + }); + + try { + // No side effects above. All conditions checked which could have + // caused us to reject this reallocation. + // COMMIT POINT + // All the effects below must succeed "atomically". Scare quotes because + // the eventual send at the bottom is part of this "atomicity" even + // though its effects happen later. The send occurs in the order of + // updates from zcf to zoe, its effects must occur immediately in zoe + // on reception, and must not fail. + // + // Commit the staged allocations (currentAllocation is replaced + // for each of the seats) and inform Zoe of the + // newAllocation. + + seats.forEach(commitStagedAllocation); + + const seatHandleAllocations = seats.map(seat => { + const seatHandle = zcfSeatToSeatHandle.get(seat); + return { seatHandle, allocation: seat.getCurrentAllocation() }; + }); + + E(zoeInstanceAdmin).replaceAllocations(seatHandleAllocations); + } catch (err) { + shutdownWithFailure(err); + throw err; + } }; /** @type {MakeZCFSeat} */ @@ -323,7 +337,7 @@ export const createSeatManager = ( return harden({ makeZCFSeat, reallocate, - reallocateInternal, + reallocateForZCFMint, dropAllReferences, }); }; diff --git a/packages/zoe/src/contractFacet/zcfZygote.js b/packages/zoe/src/contractFacet/zcfZygote.js index 22c0ee6e910..74c8fb40d8e 100644 --- a/packages/zoe/src/contractFacet/zcfZygote.js +++ b/packages/zoe/src/contractFacet/zcfZygote.js @@ -17,6 +17,7 @@ import { createSeatManager } from './zcfSeat.js'; import { makeInstanceRecordStorage } from '../instanceRecordStorage.js'; import { handlePWarning, handlePKitWarning } from '../handleWarning.js'; import { makeOfferHandlerStorage } from './offerHandlerStorage.js'; +import { addToAllocation, subtractFromAllocation } from './allocationMath.js'; import '../../exported.js'; import '../internal-types.js'; @@ -56,7 +57,7 @@ export const makeZCFZygote = ( const { makeZCFSeat, reallocate, - reallocateInternal, + reallocateForZCFMint, dropAllReferences, } = createSeatManager( zoeInstanceAdmin, @@ -152,31 +153,64 @@ export const makeZCFZygote = ( zcfSeat = makeEmptySeatKit().zcfSeat; } const totalToMint = Object.values(gains).reduce(add, empty); - zcfSeat.incrementBy(gains); + assert( + !zcfSeat.hasExited(), + `zcfSeat must be active to mint gains for the zcfSeat`, + ); + const allocationPlusGains = addToAllocation( + zcfSeat.getCurrentAllocation(), + gains, + ); + + // Increment the stagedAllocation if it exists so that the + // stagedAllocation is kept up to the currentAllocation + if (zcfSeat.hasStagedAllocation()) { + zcfSeat.incrementBy(gains); + } + // verifies offer safety - assert(zcfSeat.isOfferSafe(zcfSeat.getStagedAllocation())); - // No effects above. The following two steps - // *should* be committed atomically, but it is not a - // disaster if they are not. If we minted only, no one would - // ever get those invisibly-minted assets. + assert( + zcfSeat.isOfferSafe(allocationPlusGains), + `The allocation after minting gains ${allocationPlusGains} for the zcfSeat was not offer safe`, + ); + // No effects above, apart from incrementBy. Note COMMIT POINT within + // reallocateForZCFMint. The following two steps *should* be + // committed atomically, but it is not a disaster if they are + // not. If we minted only, no one would ever get those + // invisibly-minted assets. E(zoeMintP).mintAndEscrow(totalToMint); - // Note COMMIT POINT within reallocateInternal. - reallocateInternal([zcfSeat]); + reallocateForZCFMint(zcfSeat, allocationPlusGains); return zcfSeat; }, burnLosses: (losses, zcfSeat) => { assertAmountKeywordRecord(losses, 'losses'); const totalToBurn = Object.values(losses).reduce(add, empty); - zcfSeat.decrementBy(losses); + assert( + !zcfSeat.hasExited(), + `zcfSeat must be active to burn losses from the zcfSeat`, + ); + const allocationMinusLosses = subtractFromAllocation( + zcfSeat.getCurrentAllocation(), + losses, + ); + + // Decrement the stagedAllocation if it exists so that the + // stagedAllocation is kept up to the currentAllocation + if (zcfSeat.hasStagedAllocation()) { + zcfSeat.decrementBy(losses); + } + // verifies offer safety - assert(zcfSeat.isOfferSafe(zcfSeat.getStagedAllocation())); - // No effects above. - // Note COMMIT POINT within reallocateInternal. - // The following two steps - // *should* be committed atomically, but it is not a disaster - // if they are not. If we only commit the stagedAllocation no - // one would ever get the unburned assets. - reallocateInternal([zcfSeat]); + assert( + zcfSeat.isOfferSafe(allocationMinusLosses), + `The allocation after burning losses ${allocationMinusLosses}for the zcfSeat was not offer safe`, + ); + // No effects above, apart from decrementBy. Note COMMIT POINT within + // reallocateForZCFMint. The following two steps *should* be + // committed atomically, but it is not a disaster if they are + // not. If we only commit the allocationMinusLosses no one would + // ever get the unburned assets. + reallocateForZCFMint(zcfSeat, allocationMinusLosses); E(zoeMintP).withdrawAndBurn(totalToBurn); }, }); diff --git a/packages/zoe/src/internal-types.js b/packages/zoe/src/internal-types.js index f3dee6bd05f..8f1dbe72799 100644 --- a/packages/zoe/src/internal-types.js +++ b/packages/zoe/src/internal-types.js @@ -278,8 +278,9 @@ */ /** - * @callback ReallocateInternal - * @param {ZCFSeat[]} seats + * @callback ReallocateForZCFMint + * @param {ZCFSeat} zcfSeat + * @param {Allocation} newAllocation * @returns {void} */ @@ -295,7 +296,7 @@ * @param {ShutdownWithFailure} shutdownWithFailure * @returns {{ makeZCFSeat: MakeZCFSeat, reallocate: Reallocate, - reallocateInternal: ReallocateInternal, + reallocateForZCFMint: ReallocateForZCFMint, dropAllReferences: DropAllReferences }} */ diff --git a/packages/zoe/test/unitTests/zcf/test-reallocateForZCFMint.js b/packages/zoe/test/unitTests/zcf/test-reallocateForZCFMint.js new file mode 100644 index 00000000000..80d25980c5d --- /dev/null +++ b/packages/zoe/test/unitTests/zcf/test-reallocateForZCFMint.js @@ -0,0 +1,211 @@ +// @ts-check +// eslint-disable-next-line import/no-extraneous-dependencies +import { test } from '@agoric/zoe/tools/prepare-test-env-ava.js'; + +import { AssetKind, AmountMath } from '@agoric/ertp'; +import { makeOffer } from '../makeOffer.js'; + +import { setup } from '../setupBasicMints.js'; + +import { setupZCFTest } from './setupZcfTest.js'; + +// Test that `zcfSeat.incrementBy()` and `zcfSeat.decrementBy()` can +// be interleaved at any point with `zcfMint.mintGains()` and +// `zcfMint.burnLosses()` with no problems. This test only performs a +// subset of all possible interleavings, but it should cover a fair amount +// +// Order of calls: +// 1. zcfSeat2.decrementBy with non-zcfMint token +// 2. zcfMint.mintGains on zcfSeat2 +// 3. zcfMint.mintGains on zcfSeat1 +// 4. zcfSeat1.incrementBy non-zcfMint token +// 5. reallocate(zcfSeat1, zcfSeat2) +// 6. zcfSeat1.decrementBy zcfMint token and non-zcfMint token +// 7. zcfMint.burnLosses on zcfSeat1 +// 8. zcfMint.burnLosses on zcfSeat2 +// 9. zcfSeat2.incrementBy zcfMint token and non-zcfMint token +// 10. zcfMint.mintGains on zcfSeat2 +// 11 reallocate(zcfSeat1, zcfSeat2) + +test(`stagedAllocations safely interleave with zcfMint calls`, async t => { + const { moolaKit, moola } = setup(); + const { zoe, zcf } = await setupZCFTest({ + Moola: moolaKit.issuer, + }); + + // Make zcfSeat1 + const { zcfSeat: zcfSeat1 } = await makeOffer( + zoe, + zcf, + harden({ give: { B: moola(3) } }), + harden({ B: moolaKit.mint.mintPayment(moola(3)) }), + ); + + // Make zcfSeat2 + const { zcfSeat: zcfSeat2 } = await makeOffer( + zoe, + zcf, + harden({ give: { B: moola(3) } }), + harden({ B: moolaKit.mint.mintPayment(moola(3)) }), + ); + + // Make ZCFMint + const zcfMint = await zcf.makeZCFMint('Token', AssetKind.NAT, { + decimalPlaces: 6, + }); + const { brand: tokenBrand } = zcfMint.getIssuerRecord(); + + // Decrement zcfSeat2 by non-zcfMintToken + zcfSeat2.decrementBy({ B: moola(2) }); + t.true(zcfSeat2.hasStagedAllocation()); + t.deepEqual(zcfSeat2.getStagedAllocation(), { B: moola(1) }); + t.deepEqual(zcfSeat2.getCurrentAllocation(), { B: moola(3) }); + + // Mint gains to zcfSeat2 + zcfMint.mintGains( + { + MyToken: AmountMath.make(tokenBrand, 100n), + }, + zcfSeat2, + ); + // zcfSeat2's staged allocation and the current allocation should + // include the newly minted tokens. Staged allocations completely + // replace the old allocations, so it is important that anything + // added to the current allocation also gets added to any + // in-progress staged allocation. + t.deepEqual(zcfSeat2.getStagedAllocation(), { + B: moola(1), + MyToken: AmountMath.make(tokenBrand, 100n), + }); + t.deepEqual(zcfSeat2.getCurrentAllocation(), { + B: moola(3), + MyToken: AmountMath.make(tokenBrand, 100n), + }); + + // Mint gains to zcfSeat1 + zcfMint.mintGains( + { + OtherTokens: AmountMath.make(tokenBrand, 50n), + }, + zcfSeat1, + ); + // zcfSeat1 has no staged allocation, but the current + // allocation should have changed to include the minted tokens + t.false(zcfSeat1.hasStagedAllocation()); + t.deepEqual(zcfSeat1.getCurrentAllocation(), { + B: moola(3), + OtherTokens: AmountMath.make(tokenBrand, 50n), + }); + + // zcfSeat1.incrementBy non-zcfMint token + zcfSeat1.incrementBy({ B: moola(2) }); + // Both the staged allocation and the current allocation show the OtherTokens + t.deepEqual(zcfSeat1.getStagedAllocation(), { + B: moola(5), + OtherTokens: AmountMath.make(tokenBrand, 50n), + }); + t.deepEqual(zcfSeat1.getCurrentAllocation(), { + B: moola(3), + OtherTokens: AmountMath.make(tokenBrand, 50n), + }); + + // Reallocate + zcf.reallocate(zcfSeat1, zcfSeat2); + t.false(zcfSeat1.hasStagedAllocation()); + t.false(zcfSeat2.hasStagedAllocation()); + t.deepEqual(zcfSeat1.getCurrentAllocation(), { + B: moola(5), + OtherTokens: AmountMath.make(tokenBrand, 50n), + }); + t.deepEqual(zcfSeat2.getCurrentAllocation(), { + B: moola(1), + MyToken: AmountMath.make(tokenBrand, 100n), + }); + + // Test burnLosses interleaving + + // zcfSeat1 decrementBy both zcfMint token and non-zcfMint token + zcfSeat1.decrementBy({ + OtherTokens: AmountMath.make(tokenBrand, 5n), + B: moola(1), + }); + + // zcfMint.burnLosses on zcfSeat1 + zcfMint.burnLosses( + { OtherTokens: AmountMath.make(tokenBrand, 7n) }, + zcfSeat1, + ); + // The zcfMint losses are subtracted from both the currentAllocation and the + // stagedAllocation, but currentAllocation does not include the + // stagedAllocations, and will not until zcf.reallocate is called. + t.deepEqual(zcfSeat1.getCurrentAllocation(), { + B: moola(5), // no change since reallocate + OtherTokens: AmountMath.make(tokenBrand, 43n), + }); + t.deepEqual(zcfSeat1.getStagedAllocation(), { + B: moola(4), + OtherTokens: AmountMath.make(tokenBrand, 38n), // includes decrementBy and burnLosses + }); + + // zcfMint.burnLosses on zcfSeat2 + zcfMint.burnLosses( + { + MyToken: AmountMath.make(tokenBrand, 17n), + }, + zcfSeat2, + ); + t.deepEqual(zcfSeat2.getCurrentAllocation(), { + B: moola(1), + MyToken: AmountMath.make(tokenBrand, 83n), + }); + t.false(zcfSeat2.hasStagedAllocation()); + + // zcfSeat2.incrementBy + zcfSeat2.incrementBy({ + OtherTokens: AmountMath.make(tokenBrand, 5n), // let's keep this keyword separate even though we don't have to + B: moola(1), + }); + t.deepEqual(zcfSeat2.getCurrentAllocation(), { + B: moola(1), + MyToken: AmountMath.make(tokenBrand, 83n), + }); + t.deepEqual(zcfSeat2.getStagedAllocation(), { + B: moola(2), + MyToken: AmountMath.make(tokenBrand, 83n), + OtherTokens: AmountMath.make(tokenBrand, 5n), + }); + + // zcfMint.mintGains on zcfSeat2 + zcfMint.mintGains( + { + AnotherOne: AmountMath.make(tokenBrand, 2n), + }, + zcfSeat2, + ); + t.deepEqual(zcfSeat2.getCurrentAllocation(), { + B: moola(1), + MyToken: AmountMath.make(tokenBrand, 83n), + AnotherOne: AmountMath.make(tokenBrand, 2n), + }); + t.deepEqual(zcfSeat2.getStagedAllocation(), { + B: moola(2), + MyToken: AmountMath.make(tokenBrand, 83n), + OtherTokens: AmountMath.make(tokenBrand, 5n), + AnotherOne: AmountMath.make(tokenBrand, 2n), + }); + + // One last reallocate + zcf.reallocate(zcfSeat1, zcfSeat2); + t.deepEqual(zcfSeat2.getCurrentAllocation(), { + B: moola(2), + MyToken: AmountMath.make(tokenBrand, 83n), + OtherTokens: AmountMath.make(tokenBrand, 5n), + AnotherOne: AmountMath.make(tokenBrand, 2n), + }); + t.deepEqual(zcfSeat1.getCurrentAllocation(), { + B: moola(4), + OtherTokens: AmountMath.make(tokenBrand, 38n), + }); + t.false(zcfSeat1.hasStagedAllocation()); + t.false(zcfSeat2.hasStagedAllocation()); +}); diff --git a/packages/zoe/test/unitTests/zcf/test-zcf.js b/packages/zoe/test/unitTests/zcf/test-zcf.js index cc4ac87cf5f..17630277ba3 100644 --- a/packages/zoe/test/unitTests/zcf/test-zcf.js +++ b/packages/zoe/test/unitTests/zcf/test-zcf.js @@ -538,7 +538,7 @@ test(`zcf.makeZCFMint - mintGains - seat exited`, async t => { t.throws( () => zcfMint.mintGains({ A: AmountMath.make(4n, brand) }, zcfSeat), { - message: `seat has been exited`, + message: `zcfSeat must be active to mint gains for the zcfSeat`, }, ); }); @@ -562,7 +562,7 @@ test(`zcf.makeZCFMint - burnLosses - seat exited`, async t => { t.throws( () => zcfMint.burnLosses({ A: AmountMath.make(1n, brand) }, zcfSeat), { - message: `seat has been exited`, + message: `zcfSeat must be active to burn losses from the zcfSeat`, }, ); }); From d9eed058aebe4475531abec50d027bf3e88ba1e2 Mon Sep 17 00:00:00 2001 From: Kate Sills Date: Mon, 20 Sep 2021 16:54:18 -0700 Subject: [PATCH 2/5] chore: fix type --- packages/zoe/src/contractFacet/zcfSeat.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/zoe/src/contractFacet/zcfSeat.js b/packages/zoe/src/contractFacet/zcfSeat.js index 62027414b80..bf4fde80e4e 100644 --- a/packages/zoe/src/contractFacet/zcfSeat.js +++ b/packages/zoe/src/contractFacet/zcfSeat.js @@ -113,7 +113,7 @@ export const createSeatManager = ( * and so can be used internally for reallocations that violate * conservation. * - * @type {reallocateForZCFMint} + * @type {ReallocateForZCFMint} */ const reallocateForZCFMint = (zcfSeat, newAllocation) => { try { From 96b5ee515d0d0ad8689d4ffaf7955541f16ba830 Mon Sep 17 00:00:00 2001 From: Kate Sills Date: Mon, 20 Sep 2021 18:02:52 -0700 Subject: [PATCH 3/5] chore: specialize how seatHandleAllocations are made Co-authored-by: Mark S. Miller --- packages/zoe/src/contractFacet/zcfSeat.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/zoe/src/contractFacet/zcfSeat.js b/packages/zoe/src/contractFacet/zcfSeat.js index bf4fde80e4e..29cd45f7f40 100644 --- a/packages/zoe/src/contractFacet/zcfSeat.js +++ b/packages/zoe/src/contractFacet/zcfSeat.js @@ -129,7 +129,12 @@ export const createSeatManager = ( activeZCFSeats.set(zcfSeat, newAllocation); - const seatHandleAllocations = [zcfSeat].map(seat => { + const seatHandleAllocations = [ + { + seatHandle: zcfSeatToSeatHandle.get(zcfSeat), + allocation: newAllocation, + }, + ]; const seatHandle = zcfSeatToSeatHandle.get(seat); return { seatHandle, allocation: newAllocation }; }); From 30f5295eb27fc100cdcc36b27c0bb73a2caafdc0 Mon Sep 17 00:00:00 2001 From: Kate Sills Date: Mon, 20 Sep 2021 18:08:57 -0700 Subject: [PATCH 4/5] chore: clean up prior commit --- packages/zoe/src/contractFacet/zcfSeat.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/zoe/src/contractFacet/zcfSeat.js b/packages/zoe/src/contractFacet/zcfSeat.js index 29cd45f7f40..0e23d766684 100644 --- a/packages/zoe/src/contractFacet/zcfSeat.js +++ b/packages/zoe/src/contractFacet/zcfSeat.js @@ -135,9 +135,6 @@ export const createSeatManager = ( allocation: newAllocation, }, ]; - const seatHandle = zcfSeatToSeatHandle.get(seat); - return { seatHandle, allocation: newAllocation }; - }); E(zoeInstanceAdmin).replaceAllocations(seatHandleAllocations); } catch (err) { From 991926c90b1dc8d2c52b3a9598418c20327195fd Mon Sep 17 00:00:00 2001 From: Kate Sills Date: Mon, 20 Sep 2021 18:11:00 -0700 Subject: [PATCH 5/5] chore: add explanation for offer safety check within mintGains --- packages/zoe/src/contractFacet/zcfZygote.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/zoe/src/contractFacet/zcfZygote.js b/packages/zoe/src/contractFacet/zcfZygote.js index 74c8fb40d8e..25f15e2ffba 100644 --- a/packages/zoe/src/contractFacet/zcfZygote.js +++ b/packages/zoe/src/contractFacet/zcfZygote.js @@ -168,7 +168,10 @@ export const makeZCFZygote = ( zcfSeat.incrementBy(gains); } - // verifies offer safety + // Offer safety should never be able to be violated here, as + // we are adding assets. However, we keep this check so that + // all reallocations are covered by offer safety checks, and + // that any bug within Zoe that may affect this is caught. assert( zcfSeat.isOfferSafe(allocationPlusGains), `The allocation after minting gains ${allocationPlusGains} for the zcfSeat was not offer safe`,