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

fix: use unique offer descriptions for swaps #44

Merged
merged 3 commits into from
May 21, 2024

Conversation

samsiegart
Copy link
Contributor

Fixes #43
Refs #40

Also removes ,tx.json, will add it to .gitignore in a follow-up PR

@samsiegart samsiegart requested a review from dckc May 20, 2024 18:47
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

go ahead and make it durable?

Comment on lines 124 to 133
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...

@samsiegart samsiegart requested a review from dckc May 21, 2024 07:50
@samsiegart samsiegart merged commit 8d801d9 into main May 21, 2024
2 checks passed
@samsiegart samsiegart deleted the unique-offer-descriptions branch May 21, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swap invitation descriptions are not unique
2 participants