diff --git a/packages/zoe/src/contractFacet/zcfSeat.js b/packages/zoe/src/contractFacet/zcfSeat.js index 8709723a111..0e23d766684 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,16 +124,17 @@ 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 seatHandle = zcfSeatToSeatHandle.get(seat); - return { seatHandle, allocation: seat.getCurrentAllocation() }; - }); + const seatHandleAllocations = [ + { + seatHandle: zcfSeatToSeatHandle.get(zcfSeat), + allocation: newAllocation, + }, + ]; E(zoeInstanceAdmin).replaceAllocations(seatHandleAllocations); } catch (err) { @@ -208,8 +185,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 +339,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..25f15e2ffba 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,67 @@ export const makeZCFZygote = ( zcfSeat = makeEmptySeatKit().zcfSeat; } const totalToMint = Object.values(gains).reduce(add, empty); - 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.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); + } + + // 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`, + ); + // 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`, }, ); });