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

fix: use unique offer descriptions for swaps #44

Merged
merged 3 commits into from
May 21, 2024
Merged

fix: use unique offer descriptions for swaps #44

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7f9866
fix: use unique offer descriptions for swaps
samsiegart May 20, 2024
3b409ff
feat: store offerNonce durably
samsiegart May 21, 2024
ff44ea3
fix: lint
samsiegart May 21, 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
Empty file removed contract/,tx.json
View file
Open in desktop
Empty file.
14 changes: 13 additions & 1 deletion contract/src/swaparoo.contract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,17 @@ export const start = async (zcf, privateArgs, baggage) => {
// TODO: update with Fee param
const feeShape = makeNatAmountShape(feeBrand, params.getFee().value);

const generateOfferNonce = (() => {
// XXX: This is not stored durably, so ensure it stays unique after upgrade,
// perhaps by appending a new character each time.
let offerNonce = -1;

return () => {
offerNonce += 1;
return `${offerNonce}`;
};
})();
Copy link
Member

Choose a reason for hiding this comment

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

Since this is an example, we might as well store it durably; adding it isn't any longer than the XXX comment:

Suggested change
const generateOfferNonce = (() => {
// XXX: This is not stored durably, so ensure it stays unique after upgrade,
// perhaps by appending a new character each time.
let offerNonce = -1;
return () => {
offerNonce += 1;
return `${offerNonce}`;
};
})();
const generateOfferNonce = (() => {
let offerNonce = provide(baggage, 'offerNonce', () => -1);
return () => {
offerNonce += 1;
baggage.set('offerNonce', offerNonce);
return `${offerNonce}`;
};
})();

along with:

import { provide } from '@agoric/vat-data';

Copy link
Member

Choose a reason for hiding this comment

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

Also, we might as well return offerNonce, since it gets stringified later.

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. Just writing out my discovery process for reference (tried not going straight to your hints):

  • Looked for other durable examples in dapp-agoric-basics, found makeDurableGovernorFacet, doesn't seem like quite what I want.
  • Go to docs.agoric.com and search "durability", find https://docs.agoric.com/guides/zoe/contract-upgrade.html#durability
  • See an example with provide and makeScalarBigMapStore. I just need a number, not a map, not sure if that's allowed, but from your example it apparently is.
  • yarn add @agoric/vat-data and use the provide method -> Tests failing, seems like it brought in some conflicting dependency versions.
  • Nuke node_modules, remove @agoric/vat-data, do another yarn install and then do yarn why @agoric/vat-data to see which version is already transitively depended on.
  • Add that version to package.json, now only one test case failing:
build-proposal › bundles from build:deployer meet 1MB request limit
  test/test-build-proposal.js:18

   17:     const rootPath = undefined; // not needed since bundle is already cached
   18:     const bundle = await bundleCache.load(rootPath, name);
   19:     const fileContents = JSON.stringify(bundle);          

  Rejected promise returned by test. Reason:

  TypeError {
    code: 'ERR_INVALID_ARG_TYPE',
    message: 'The "paths[1]" argument must be of type string. Received undefined',
  }
  • Some hint about bundle caching there, run yarn build before yarn test again and now it passes.

Copy link
Member

Choose a reason for hiding this comment

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

... writing out my discovery process...

nice!

  • yarn add @agoric/vat-data and use the provide method -> Tests failing, seems like it brought in some conflicting dependency versions.
  • Nuke node_modules, remove @agoric/vat-data, do another yarn install and then do yarn why @agoric/vat-data to see which version is already transitively depended on.

Yes, adding dependencies is incredibly fragile. I'm tracking that as...


/**
* @param { ZCFSeat } firstSeat
* @param {{ addr: string }} offerArgs
Expand Down Expand Up @@ -158,9 +169,10 @@ export const start = async (zcf, privateArgs, baggage) => {
return swapWithFee(zcf, firstSeat, secondSeat, feeSeat, params.getFee());
};

const description = `matchOffer-${generateOfferNonce()}`;
const secondSeatInvitation = await zcf.makeInvitation(
secondSeatOfferHandler,
'matchOffer',
description,
{ give: give1, want: want1 }, // "give" and "want" are from the proposer's perspective
);

Expand Down
2 changes: 1 addition & 1 deletion contract/test/snapshots/test-swap-wallet.js.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ Generated by [AVA](https://avajs.dev).
{
id: 'jack-123',
invitationSpec: {
description: 'matchOffer',
description: 'matchOffer-0',
instance: Object @Alleged: InstanceHandle {},
source: 'purse',
},
Expand Down
Binary file modified contract/test/snapshots/test-swap-wallet.js.snap
View file
Open in desktop
Binary file not shown.
13 changes: 11 additions & 2 deletions contract/test/test-swap-wallet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ const startAlice = async (
* @param {MockWallet} wallet
* @param {Amount} beansAmount
* @param {Amount} cowsAmount
* @param {string} offerDescription
* @param {boolean} [jackPays]
*/
const startJack = async (
Expand All @@ -169,6 +170,7 @@ const startJack = async (
wallet,
beansAmount,
cowsAmount,
offerDescription,
jackPays = false,
) => {
const instance = wellKnown.instance[contractName];
Expand All @@ -188,7 +190,7 @@ const startJack = async (
invitationSpec: {
source: 'purse',
instance,
description: 'matchOffer',
description: offerDescription,
},
proposal,
};
Expand Down Expand Up @@ -278,7 +280,14 @@ test.serial('basic swap', async t => {
await E(E.get(bldIssuerKit).mint).mintPayment(cowAmount),
);
const jackSeat = seatLike(
await startJack(t, wellKnown, wallet.jack, fiveBeans, cowAmount),
await startJack(
t,
wellKnown,
wallet.jack,
fiveBeans,
cowAmount,
'matchOffer-0',
),
);

const jackPayouts = await jackSeat.getPayoutAmounts();
Expand Down
2 changes: 1 addition & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8453,7 +8453,7 @@ eslint@^8.47.0, eslint@^8.56.0:
strip-ansi "^6.0.1"
text-table "^0.2.0"

esm@agoric-labs/esm#Agoric-built, "esm@github:agoric-labs/esm#Agoric-built":
esm@agoric-labs/esm#Agoric-built:
version "3.2.25"
resolved "https://codeload.github.com/agoric-labs/esm/tar.gz/3603726ad4636b2f865f463188fcaade6375638e"

Expand Down