-
Notifications
You must be signed in to change notification settings - Fork 206
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 issuerKeywordRecord and brandKeywordRecord to terms #1459
Conversation
4020981
to
53693f3
Compare
9729895
to
173d03d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely better!
LGTM
* @property {AmountMathKeywordRecord} terms.maths - record with keywords | ||
* keys, amountMath values | ||
* | ||
* @typedef {StandardTerms & Record<string, any>} Terms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you want a conjunctive type here, but rather an extends
. But this remains beyond my expertise. @michaelfig ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was recommended by Michael a week or so ago. It may be outdated :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, extends
is not available in JSDoc, so intersection it is!
await getPromiseForIssuerRecords(issuers); | ||
|
||
const allSeatStagings = new WeakSet(); | ||
|
||
const registerIssuerRecord = (keyword, issuerRecord) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why separate this out from registerIssuerRecordWKeyword
, which has the error checks, and then call this non-checking one from line 89?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because when I'm overwriting the issuerRecord from Zoe with issuerRecords that include localAmountMath, the keywords will not be unique.
|
||
const issuerRecords = await getPromiseForIssuerRecords(issuers); | ||
issuerRecords.forEach((issuerRecord, i) => { | ||
registerIssuerRecord(keywords[i], issuerRecord); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only use of the non-checking form. Why not registerIssuerRecordWKeyword
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. This loop doesn't correspond to anything in the previous code. Was omitting it an undetected bug that this fixes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, with the addition of amountMath in the terms, we need to overwrite with local amountMath.
* refactor: refactor Zoe to use seat objects rather than offerHandles * refactor: update seat namings, add seats.md to docs with diagrams * refactor: move up init of instance, instanceAdmin * fix: minting allocations in zcf (#1382) * refactor: tests for sellItems and mintAndSellNFT pass - need to be reworked for new synchronous mint still * refactor: fix jsdoc to recognize makeZCFMint as property * fix: register the new minty issuer on zcf side (#1392) * refactor: redirect main in package.json * refactor: apply zoe renames - `getInviteIssuer` to `getInvitationIssuer` - `inviteDesc` to `description` * fix: solve some easy types * fix: use local amountMath (#1408) * fix(swingset): check promise resolution table during comms.inbound refs #1400 * refactor: multipoolAutoswap * refactor: remove BrandName parameter from ERTP types * feat: add the ability for a contract to get a synchronous seat zcf.addEmptySeat() would return a zcfSeat that the contract could use to hold allocations. untested * refactor: cleanup synchronous ZCF seat creation drop keyword parameter in addEmptySeat() extract makeZoeSeatAdminKit, use it for emptySeat, too. rename zcf seat construction to use 'Kit' * refactor: use updateFromNotifier from library * refactor: add types to reveal the wallet changes needed for Zoe * refactor: post review cleanups; mostly type info improved type annotations rename makeEmptySeat to makeOfferlessSeat don't return the zcfSeatAdmin to the contract * fix: make Zoe typesafe again * refactor: bring autoswap up to date on the new Zoe spike branch Based on the synchronous seat work. The unit tests for autoswap pass. I haven't yet done the swingset tests. * fix: update autoswap from addEmptySeat() to makeEmptySeatKit() * chore: add publicFacet and instance to MakeInstanceResult (#1418) * chore: fix typing of MakeInstanceResult * chore: add hasExited, getNotifier to UserSeat * Update more unit tests (#1422) * chore: update tests * chore: finish automaticRefund tests * chore: update barterExchange and brokenContract tests. Small changes to Zoe and types (#1427) * fix: make the wallet pass unit tests * refactor: clear up some more types * fix(dapp-svelte-wallet): minor cleanups * fix: always use the published wallet payment facet * test: invite -> creatorInvitation * fix: noticed bug in exit code while doing (chore: update coveredCall tests) (#1428) * chore: update tests for escrowToVote, grifter, multipoolAutoswap, publicAuction, sellTickets, simpleExchange * chore: make more unit tests pass, linting * chore: update tests for escrowToVote, grifter, multipoolAutoswap, publicAuction, sellTickets, simpleExchange (#1430) * fix: minor wallet cleanup * test: fix all the zoe swingSet tests * chore: find usages of .getBrand() and make sure that brand and issuer match (#1443) * refactor: rename makeInstance to startInstance (#1444) * test(dapp-svelte-wallet): fix test log for determinism The prior golden test log relied on a race between deposit and the new purse values. Correct that ambiguity. * fix: generate unique petnames, and use them * fix: only uniquify suffixes for petnames that are actually paths * chore: fix brokenContracts tests, zoe-metering tests, skip ZoeHelpers tests for now (need to be rewritten entirely, I think using the real ZCF, not a mock) (#1435) * fix: minor typing issue * chore: clean up vestiges of encouragement dapp * chore: remove contract/deploy.js from the wallet spec * refactor: prevent the same seat being an argument multiple times in reallocate (#1452) * refactor: remove makeEmptyOffer from zoeHelpers, use zcf.makeEmptySeatKit instead (#1451) * refactor: move issuerKeywordRecord and brandKeywordRecord to terms (#1459) * refactor: add issuers, brands, maths to terms * refactor: rename NonCustomTerms in types * chore: address PR comments * refactor: move AmountMath from names to Kinds; easier local creation Issuer no longer has getAmountMath(), instead exports makeLocalAmountMath(); added an enum for the three kinds of AmountMath getAmountMathHelpersName replaced with getAmountMathKind Add MathKind as an enum containing the legal values of AmountKind Made progress on correct typing * fix: review suggestions and merge conflicts. * fix: clear up the types * chore: export all of ERTP from index.js. Update all imports. A few other review-suggested cleanups * refactor: add Zoe methods for getting assured invitation values The type declarations seem to work. * refactor: drop unnecessary invitations in the metering tests. * refactor: review of the following files in the form of code changes (#1475) * refactor: review of: packages/zoe/src/contracts/autoswap.js packages/zoe/src/contracts/barterExchange.js packages/zoe/src/contracts/simpleExchange.js packages/zoe/test/swingsetTests/zoe/test-zoe.js packages/zoe/test/swingsetTests/zoe/vat-alice.js packages/zoe/test/swingsetTests/zoe/vat-bob.js packages/zoe/test/swingsetTests/zoe/vat-carol.js packages/zoe/test/swingsetTests/zoe/vat-dave.js packages/zoe/test/swingsetTests/zoe/vat-zoe.js packages/zoe/test/unitTests/contracts/test-autoswap.js packages/zoe/test/unitTests/contracts/test-barter.js packages/zoe/test/unitTests/contracts/test-simpleExchange.js packages/zoe/test/unitTests/installFromSource.js packages/zoe/test/unitTests/setupBasicMints.js packages/zoe/test/unitTests/setupMixedMints.js packages/zoe/test/unitTests/setupNonFungibleMints.js packages/zoe/test/unitTests/test-cleanProposal.js packages/zoe/test/unitTests/test-offerSafety.js packages/zoe/test/unitTests/test-rightsConservation.js packages/zoe/test/zoeTestHelpers.js in PR form rather than comments * fix: fix dapp-svelte-wallet issues * fix: lint-fix * fix: fix errors * chore: review in the form of PR w/ changes (#1484) * fix: resolve semantic merge conflict The `@agoric/zoe/src/contractFacet` moved to: `@agoric/zoe/src/contractFacet/contractFacet.js`. * refactor: add typings for some sample Zoe contracts * refactor: loosen unknown typings * chore: expose '@agoric/zoe/contractFacet' * docs: address review comments * test(swingset-runner): update demo/exchangeBenchmark for new Zoe * chore: follow #1194: take fresh copy of zoeTests from zoe * fix: don't use monorepo-relative paths to @agoric/zoe * chore: complete #1194 Problem 1 * fix: zcf is unused, so prepend an underscore lint fix * chore: complete #1194 Problem 2b * chore: complete #1194 Problem 2c * chore: move global harden declaration to package.json * fix: undo invalid Zoe change * fix: minor cleanups * chore: rename userSeat.exit() to tryExit() (#1493) * chore: rename userSeat.exit() to tryExit() * Update packages/zoe/src/objArrayConversion.js Co-authored-by: Mark S. Miller <[email protected]> * chore: address small PR comments * chore: remove objToArray because unused * chore: move cleanProposal.js (#1495) * chore: remove more unused functions * chore: remove escrowAndAllocateTo * chore: remove unused type * chore: rename invite to invitation, forever more. begone! * Use flatMap Co-authored-by: Mark S. Miller <[email protected]> * chore: change areRightsConserved to assertRightsConserved * chore: address PR comments * fix: reintroduce makeZoe's zcfBundleName argument; lost in merge * Update packages/zoe/src/zoeService/zoeSeat.js Co-authored-by: Chris Hibbert <[email protected]> * chore: address PR comments * chore: fix the lint * chore: address PR comments about kick out messages (#1529) * chore: address PR comments about exit behavior (#1527) * chore: address PR comments about exit behavior * Update packages/zoe/src/zoeService/zoeSeat.js Co-authored-by: Mark S. Miller <[email protected]> * chore: fix lint, slight errors from merge Co-authored-by: Mark S. Miller <[email protected]> Co-authored-by: Mark S. Miller <[email protected]> Co-authored-by: Dean Tribble <[email protected]> Co-authored-by: Michael FIG <[email protected]> Co-authored-by: Brian Warner <[email protected]> Co-authored-by: Chris Hibbert <[email protected]> Co-authored-by: Chris Hibbert <[email protected]>
Move issuerKeywordRecord and brandKeywordRecord to terms and rename as issuers and brands, respectively
Closes #1455