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.

Sign up for GitHub

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: use named fields in atomicRearrange #65

Open
wants to merge 5 commits into
base: dc-narrow-powers
Choose a base branch
from
+44 −10
Open

refactor: use named fields in atomicRearrange #65

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
65d8ef6
docs: explain platform-goals/
dckc Feb 14, 2024
fd8d9ec
feat: atomicRearrange with named field support
dckc Feb 14, 2024
593e65c
refactor: use named fields in atomicRearrange
dckc Feb 14, 2024
6df24ef
chore: refine names, add mappedTo in TransferPartRecord
dckc Feb 15, 2024
04f7376
refactor: slightly more concise
dckc Feb 15, 2024
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
16 changes: 6 additions & 10 deletions contract/src/offer-up.contract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { Far } from '@endo/far';
import { M, getCopyBagEntries } from '@endo/patterns';
import { AssetKind } from '@agoric/ertp/src/amountMath.js';
import { AmountShape } from '@agoric/ertp/src/typeGuards.js';
import { atomicRearrange } from '@agoric/zoe/src/contractSupport/atomicTransfer.js';
import { atomicRearrange } from './platform-goals/zcfTools.js';
import '@agoric/zoe/exported.js';

const { Fail, quote: q } = assert;
Expand Down Expand Up @@ -107,15 +107,11 @@ export const start = async zcf => {
Fail`max ${q(maxItems)} items allowed: ${q(want.Items)}`;

const newItems = itemMint.mintGains(want);
atomicRearrange(
zcf,
harden([
// price from buyer to proceeds
[buyerSeat, proceeds, { Price: tradePrice }],
// new items to buyer
[newItems, buyerSeat, want],
]),
);
const charge = { Price: tradePrice };
atomicRearrange(zcf, [
Copy link
Member

Choose a reason for hiding this comment

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

Please restore the harden

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, already stated. (I thought I was commenting on another PR where I hadn't yet said this.)

{ from: buyerSeat, to: proceeds, amounts: charge },
{ from: newItems, to: buyerSeat, amounts: want },
]);

buyerSeat.exit(true);
newItems.exit();
Expand Down
5 changes: 5 additions & 0 deletions contract/src/platform-goals/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Platform Goals

Work on this dapp shows some ways that the platform could be improved.

The code in this directory shows some possible platform refinements.
34 changes: 34 additions & 0 deletions contract/src/platform-goals/zcfTools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// @ts-check
import { atomicRearrange as atomicRearrangeTuples } from '@agoric/zoe/src/contractSupport/atomicTransfer.js';
import { mapKeywords } from '@agoric/zoe/src/contractSupport/zoeHelpers.js';

/** @typedef {import("@agoric/zoe/src/contractSupport/atomicTransfer").TransferPart} TransferPart */

/**
* Refine atomicRearrange to support naming the fields of transfer parts.
*
* @param {ZCF} zcf
* @param { Array<TransferPartRecord | TransferPart>} transferParts
*
* @typedef {{
* from?: ZCFSeat,
* to?: ZCFSeat,
* amounts?: AmountKeywordRecord,
* mappedTo?: KeywordKeywordRecord
Copy link
Member

@erights erights Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am surprised by this. When I saw @dtribble 's suggestion that the optional fourth field be named mappedTo, it had not occurred to me that it should be a KeywordKeywordRecord for mapping the entries in amounts. I was just thinking that it was an alternative to the name toAmounts.

On first blush, both are plausble. As an AmountKeywordRecord, the original meaning of toAmounts (whatever it is called) is more expressive, but perhaps not usefully so. Independent of naming, what are y'all's thoughts on which is the better API design?

Btw, when I said mappedTo grew on me, I only had in mind that it was just a renaming of the original fourth AmountKeywordRecord optional argument. But the possibility of having it be a KeywordKeywordRecord had not occurred to me. Given that this choice also makes sense, I also agree that the name mappedTo is only a good name if we choose the KeywordKeywordRecord meaning for it.

Copy link
Member

Choose a reason for hiding this comment

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

I also don't know which semantics @dtribble had in mind when he suggested mappedTo. @dtribble ?

Copy link

Choose a reason for hiding this comment

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

Given that the amounts must match, the KeywordKeywordRecord has the virtue of not requiring redundant specification, but it also means you can't split up the amounts differently. I think cases where you need to do that are rare, and can be covered in fromOnly and toOnly.

* }} TransferPartRecord
*/
export const atomicRearrange = (zcf, transferParts) => {
/** @type {TransferPart[]} */
const tuples = harden(
transferParts.map(part => {
if (Array.isArray(part)) {
return part;
}
const toAmounts = part.mappedTo
? mapKeywords(part.amounts, part.mappedTo)
: undefined;
return [part.from, part.to, part.amounts, toAmounts];
}),
);
return atomicRearrangeTuples(zcf, tuples);
};
Loading