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

refactor: move atomicRearrange into Zcf #7900

Merged
merged 7 commits into from
Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/inter-protocol/src/auction/auctioneer.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ const distributeProportionalShares = (

const collShare = makeRatioFromAmounts(unsoldCollateral, totalCollDeposited);
const currShare = makeRatioFromAmounts(proceeds, totalCollDeposited);
/** @type {import('@agoric/zoe/src/contractSupport/atomicTransfer.js').TransferPart[]} */
/** @type {TransferPart[]} */
const transfers = [];
let proceedsLeft = proceeds;
let collateralLeft = unsoldCollateral;
Expand Down Expand Up @@ -255,7 +255,7 @@ export const distributeProportionalSharesWithLimits = (
// collateral to reach their share. Then see what's left, and allocate it
// among the remaining depositors. Escape to distributeProportionalShares if
// anything doesn't work.
/** @type {import('@agoric/zoe/src/contractSupport/atomicTransfer.js').TransferPart[]} */
/** @type {TransferPart[]} */
const transfers = [];
let proceedsLeft = proceeds;
let collateralLeft = unsoldCollateral;
Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/vaultFactory/liquidation.js
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ export const getLiquidatableVaults = (
const { zcfSeat: liqSeat } = zcf.makeEmptySeatKit();
let totalDebt = AmountMath.makeEmpty(debtBrand);
let totalCollateral = AmountMath.makeEmpty(collateralBrand);
/** @type {import('@agoric/zoe/src/contractSupport/atomicTransfer.js').TransferPart[]} */
/** @type {TransferPart[]} */
const transfers = [];

for (const vault of vaultsToLiquidate.values()) {
Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/vaultFactory/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
* @param {ZCFSeat} mintReceiver
* @param {Amount<'nat'>} toMint
* @param {Amount<'nat'>} fee
* @param {import('@agoric/zoe/src/contractSupport/atomicTransfer.js').TransferPart[]} transfers
* @param {TransferPart[]} transfers
* @returns {void}
*/

Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/vaultFactory/vault.js
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,7 @@ export const prepareVault = (baggage, makeRecorderKit, zcf) => {

const giveMintedTaken = AmountMath.subtract(fp.give.Minted, surplus);

/** @type {import('@agoric/zoe/src/contractSupport/atomicTransfer.js').TransferPart[]} */
/** @type {TransferPart[]} */
const transfers = harden([
[clientSeat, vaultSeat, { Collateral: fp.give.Collateral }],
[vaultSeat, clientSeat, { Collateral: fp.want.Collateral }],
Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/vaultFactory/vaultDirector.js
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ const prepareVaultDirector = (
mintAndTransfer: (mintReceiver, toMint, fee, nonMintTransfers) => {
const kept = AmountMath.subtract(toMint, fee);
debtMint.mintGains(harden({ Minted: toMint }), mintSeat);
/** @type {import('@agoric/zoe/src/contractSupport/atomicTransfer.js').TransferPart[]} */
/** @type {TransferPart[]} */
const transfers = [
...nonMintTransfers,
[mintSeat, rewardPoolSeat, { Minted: fee }],
Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/vaultFactory/vaultManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,7 @@ export const prepareVaultManagerKit = (
if (plan.transfersToVault.length > 0) {
const transfers = plan.transfersToVault.map(
([vaultIndex, amounts]) =>
/** @type {import('@agoric/zoe/src/contractSupport/atomicTransfer.js').TransferPart} */ ([
/** @type {TransferPart} */ ([
liqSeat,
vaultsInPlan[vaultIndex].getVaultSeat(),
amounts,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
/** @file DEPRECATED use the vault test driver instead */
import { AmountMath, makeIssuerKit } from '@agoric/ertp';

import { assert } from '@agoric/assert';
import { makePublishKit, observeNotifier } from '@agoric/notifier';
import {
makeFakeMarshaller,
makeFakeStorage,
} from '@agoric/notifier/tools/testSupports.js';
import {
atomicRearrange,
prepareRecorderKit,
unitAmount,
} from '@agoric/zoe/src/contractSupport/index.js';
Expand Down Expand Up @@ -101,14 +99,14 @@ export async function start(zcf, privateArgs, baggage) {
const mintAndTransfer = (mintReceiver, toMint, fee, nonMintTransfers) => {
const kept = AmountMath.subtract(toMint, fee);
stableMint.mintGains(harden({ Minted: toMint }), mintSeat);
/** @type {import('@agoric/zoe/src/contractSupport/atomicTransfer.js').TransferPart[]} */
/** @type {TransferPart[]} */
const transfers = [
...nonMintTransfers,
[mintSeat, vaultFactorySeat, { Minted: fee }],
[mintSeat, mintReceiver, { Minted: kept }],
];
try {
atomicRearrange(zcf, harden(transfers));
zcf.atomicRearrange(harden(transfers));
} catch (e) {
console.error('mintAndTransfer caught', e);
stableMint.burnLosses(harden({ Minted: toMint }), mintSeat);
Expand Down
1 change: 1 addition & 0 deletions packages/zoe/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@
"test/unitTests/zcf/test-reallocate-empty.js",
"test/unitTests/zcf/test-zoeHelpersWZcf.js",
"test/unitTests/zcf/test-reallocateForZCFMint.js",
"test/unitTests/zcf/test-atomicRearrange.js",
"test/unitTests/zcf/test-zcf.js",
"test/unitTests/zcf/test-allStagedSeatsUsed.js",
"# ManualTimer.setWakeup: no function",
Expand Down
94 changes: 94 additions & 0 deletions packages/zoe/src/contractFacet/reallocate.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import { makeScalarMapStore } from '@agoric/vat-data';

import { assertRightsConserved } from './rightsConservation.js';
import { addToAllocation, subtractFromAllocation } from './allocationMath.js';

const { Fail } = assert;

/** @typedef {Array<AmountKeywordRecord>} TransactionList */

/**
* Convert from a list of transfer descriptions ([fromSeat, toSeat, fromAmount,
* toAmount], with many parts optional) to a list of resulting allocations for
* each of the seats mentioned.
*
* @param {Array<TransferPart>} transfers
* @returns {[ZCFSeat, AmountKeywordRecord][]}
*/
export const makeAllocationMap = transfers => {
/** @type {MapStore<ZCFSeat, [TransactionList, TransactionList]>} */
const allocations = makeScalarMapStore();

const getAllocations = seat => {
if (allocations.has(seat)) {
return allocations.get(seat);
}

/** @type {[TransactionList, TransactionList]} */
const pair = [[], []];
allocations.init(seat, pair);
return pair;
};

const decrementAllocation = (seat, decrement) => {
const [incr, decr] = getAllocations(seat);

const newDecr = [...decr, decrement];
allocations.set(seat, [incr, newDecr]);
};

const incrementAllocation = (seat, increment) => {
const [incr, decr] = getAllocations(seat);

const newIncr = [...incr, increment];
allocations.set(seat, [newIncr, decr]);
};

for (const [fromSeat, toSeat, fromAmounts, toAmounts] of transfers) {
if (fromSeat) {
if (!fromAmounts) {
throw Fail`Transfer from ${fromSeat} must say how much`;
}
decrementAllocation(fromSeat, fromAmounts);
if (toSeat) {
// Conserved transfer between seats
if (toAmounts) {
// distinct amounts, so we check conservation.
assertRightsConserved(
Object.values(fromAmounts),
Object.values(toAmounts),
);
incrementAllocation(toSeat, toAmounts);
} else {
// fromAmounts will be used for toAmounts as well
incrementAllocation(toSeat, fromAmounts);
}
} else {
// Transfer only from fromSeat
!toAmounts ||
Fail`Transfer without toSeat cannot have toAmounts ${toAmounts}`;
}
} else {
toSeat || Fail`Transfer must have at least one of fromSeat or toSeat`;
// Transfer only to toSeat
!fromAmounts ||
Fail`Transfer without fromSeat cannot have fromAmounts ${fromAmounts}`;
toAmounts || Fail`Transfer to ${toSeat} must say how much`;
incrementAllocation(toSeat, toAmounts);
}
}

/** @type {[ZCFSeat,AmountKeywordRecord][]} */
const resultingAllocations = [];
Copy link
Member

Choose a reason for hiding this comment

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

resultingAllocations is a different shape than allocations. consider renaming to resultingEntries or resolvedEntries.

consider a functional style using a transform function:

  const resolveAllocation = (seat, [incrList, decrList]) => {};
  const resolvedEntries = allocations.entries().map(resolveAllocation);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I looked at that, but allocations.entries() isn't an array. Using Array.from() would require declaring more types.

for (const [seat, [incrList, decrList]] of allocations.entries()) {
let newAlloc = seat.getCurrentAllocation();
for (const incr of incrList) {
newAlloc = addToAllocation(newAlloc, incr);
}
for (const decr of decrList) {
newAlloc = subtractFromAllocation(newAlloc, decr);
}
turadg marked this conversation as resolved.
Show resolved Hide resolved
resultingAllocations.push([seat, newAlloc]);
}
return resultingAllocations;
};
12 changes: 12 additions & 0 deletions packages/zoe/src/contractFacet/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
* synchronously from within the contract, and usually is referred to
* in code as zcf.
*
* @property {(transfers: TransferPart[]) => void} atomicRearrange - atomically reallocate amounts among seats.
* @property {Reallocate} reallocate - reallocate amounts among seats.
* Deprecated: Use atomicRearrange instead.
* @property {(keyword: Keyword) => void} assertUniqueKeyword - check
Expand Down Expand Up @@ -59,6 +60,8 @@
*/

/**
* @deprecated reallocate(). Use zcf.atomicRearrange() instead
*
* @typedef {(seat1: ZCFSeat, seat2: ZCFSeat, ...seatRest:
* Array<ZCFSeat>) => void} Reallocate
*
Expand All @@ -83,6 +86,15 @@
* effect offer safety for seats whose allocations change.
*/

/**
* @typedef {[
* fromSeat?: ZCFSeat,
* toSeat?: ZCFSeat,
* fromAmounts?: AmountKeywordRecord,
* toAmounts?: AmountKeywordRecord
* ]} TransferPart
*/

/**
* @callback SaveIssuer
*
Expand Down
104 changes: 101 additions & 3 deletions packages/zoe/src/contractFacet/zcfSeat.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import {
makeScalarBigWeakMapStore,
provideDurableMapStore,
provideDurableWeakMapStore,
prepareExoClass,
prepareExoClassKit,
provide,
prepareExoClass,
provideDurableMapStore,
provideDurableWeakMapStore,
} from '@agoric/vat-data';
import { E } from '@endo/eventual-send';
import { AmountMath } from '@agoric/ertp';
Expand All @@ -19,6 +19,8 @@ import {
SeatDataShape,
SeatShape,
} from '../typeGuards.js';
import { makeAllocationMap } from './reallocate.js';
import { TransferPartShape } from '../contractSupport/atomicTransfer.js';

const { Fail } = assert;

Expand Down Expand Up @@ -214,6 +216,10 @@ export const createSeatManager = (

return isOfferSafe(state.proposal, reallocation);
},
/**
* @deprecated switch to zcf.atomicRearrange()
Copy link
Member

@turadg turadg Jun 9, 2023

Choose a reason for hiding this comment

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

This is good but note that consumers of this object aren't getting the type written here. They get ZCFSeat which has the problem discussed above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. The comment is mostly for those reading/editing this file. This still seemed the most concise way to express it.

* @param {AmountKeywordRecord} amountKeywordRecord
*/
incrementBy(amountKeywordRecord) {
const { self } = this;
assertActive(self);
Expand All @@ -227,6 +233,10 @@ export const createSeatManager = (
);
return amountKeywordRecord;
},
/**
* @deprecated switch to zcf.atomicRearrange()
* @param {AmountKeywordRecord} amountKeywordRecord
*/
decrementBy(amountKeywordRecord) {
const { self } = this;
assertActive(self);
Expand Down Expand Up @@ -265,6 +275,7 @@ export const createSeatManager = (
const ZcfSeatManagerIKit = harden({
seatManager: M.interface('ZcfSeatManager', {
makeZCFSeat: M.call(SeatDataShape).returns(M.remotable('zcfSeat')),
atomicRearrange: M.call(M.arrayOf(TransferPartShape)).returns(),
reallocate: M.call(M.remotable('zcfSeat'), M.remotable('zcfSeat'))
.rest(M.arrayOf(M.remotable('zcfSeat')))
.returns(),
Expand All @@ -289,6 +300,93 @@ export const createSeatManager = (
return zcfSeat;
},

/**
turadg marked this conversation as resolved.
Show resolved Hide resolved
* Rearrange the allocations according to the transfer descriptions.
* This is a set of changes to allocations that must satisfy several
* constraints. If these constraints are all met, then the reallocation
* happens atomically. Otherwise, it does not happen at all.
*
* The conditions
* * All the mentioned seats are still live,
* * No outstanding stagings for any of the mentioned seats. Stagings
* have been deprecated in favor or atomicRearrange. To prevent
* confusion, for each reallocation, it can only be expressed in
* the old way or the new way, but not a mixture.
* * Offer safety
* * Overall conservation
*
* The overall transfer is expressed as an array of `TransferPart`. Each
* individual `TransferPart` is one of
* - A transfer from a `fromSeat` to a `toSeat`. Specify both toAmount
* and fromAmount to change keywords, otherwise only fromAmount is required.
* - A taking from a `fromSeat`'s allocation. See the `fromOnly` helper.
* - A giving into a `toSeat`'s allocation. See the `toOnly` helper.
*
* @param {TransferPart[]} transfers
*/
atomicRearrange(transfers) {
const newAllocations = makeAllocationMap(transfers);

// ////// All Seats are active /////////////////////////////////
for (const [seat] of newAllocations) {
assertActive(seat);
!seat.hasStagedAllocation() ||
Fail`Cannot mix atomicRearrange with seat stagings: ${seat}`;
zcfSeatToSeatHandle.has(seat) ||
Fail`The seat ${seat} was not recognized`;
}

// ////// Ensure that rights are conserved overall /////////////

// convert array of keywordAmountRecords to 1-level array of Amounts
const flattenAmounts = allocations =>
allocations.flatMap(Object.values);
const previousAmounts = flattenAmounts(
newAllocations.map(([seat]) => seat.getCurrentAllocation()),
);
const newAmounts = flattenAmounts(
newAllocations.map(([_, allocation]) => allocation),
);
assertRightsConserved(previousAmounts, newAmounts);

// ////// Ensure that offer safety holds ///////////////////////
for (const [seat, allocation] of newAllocations) {
isOfferSafe(seat.getProposal(), allocation) ||
Fail`Offer safety was violated by the proposed allocation: ${allocation}. Proposal was ${seat.getProposal()}`;
}

const seatHandleAllocations = newAllocations.map(
([seat, allocation]) => {
const seatHandle = zcfSeatToSeatHandle.get(seat);
return { allocation, seatHandle };
},
);
try {
// No side effects above. All conditions checked which could have
// caused us to reject this reallocation. Notice that the current
// allocations are captured in seatHandleAllocations, so there must
// be no awaits between that assignment and here.
//
// COMMIT POINT
//
// The effects must succeed atomically. The call to
// replaceAllocations() will be processed in the order of updates
// from ZCF to Zoe. Its effects must occur immediately in Zoe on
// reception, and must not fail.
//
// Commit the new allocations (currentAllocation is replaced
// for each of the seats) and inform Zoe of the new allocation.

for (const [seat, allocation] of newAllocations) {
activeZCFSeats.set(seat, allocation);
}

E(zoeInstanceAdmin).replaceAllocations(seatHandleAllocations);
Copy link
Member

Choose a reason for hiding this comment

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

this is a promise that may reject. if so it will fail silently.

I see that reallocate had this same problem. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have tried to ensure that ZCF has the authoritative word here. If ZCF and Zoe lose contact then things are very broken and will shortly fall apart.

Short of that, Zoe and ZCF are in close communication and we have tried to ensure that the two sides remain in sync, and for every shared bit of info, we know which side is authoritative.

Copy link
Member

@turadg turadg Jul 21, 2023

Choose a reason for hiding this comment

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

Ok. Please put void and a comment with that justification. In particular, why a failure of replaceAllocations shouldn't be caught by the catch and shutdownWithFailure.

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

} catch (err) {
shutdownWithFailure(err);
throw err;
}
},
reallocate(/** @type {ZCFSeat[]} */ ...seats) {
seats.forEach(assertActive);
seats.forEach(assertStagedAllocation);
Expand Down
1 change: 1 addition & 0 deletions packages/zoe/src/contractFacet/zcfZygote.js
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ export const makeZCFZygote = async (
// accept raw functions. assert cannot be a valid passable! (It's a function
// and has members.)
const zcf = Remotable('Alleged: zcf', undefined, {
atomicRearrange: transfers => seatManager.atomicRearrange(transfers),
reallocate: (...seats) => seatManager.reallocate(...seats),
assertUniqueKeyword: kwd => getInstanceRecHolder().assertUniqueKeyword(kwd),
saveIssuer: async (issuerP, keyword) => {
Expand Down
Loading
Loading