fix: document contract; refactor for clarity; use defensive patterns #49
Conversation
contract/src/offer-up.contract.js
Outdated
| * The `Items` amount must use the `Item` brand and a bag value. | ||
| */ | ||
| const proposalShape = harden({ | ||
| give: { Price: { brand: tradePrice.brand, value: M.any() } }, |
There was a problem hiding this comment.
I think you can use M.gte() here and not need AmountMath.isGTE() below
| give: { Price: { brand: tradePrice.brand, value: M.any() } }, | |
| give: { Price: { brand: tradePrice.brand, value: M.gte(price.amount) } }, |
or
| give: { Price: { brand: tradePrice.brand, value: M.any() } }, | |
| give: { Price: M.gte(price) }, |
| { maxItems: M.bigint() }, | ||
| ), | ||
| }; | ||
| // compatibility with an earlier contract metadata API |
There was a problem hiding this comment.
What does this mean? I have not seen meta.customTermsShape before.
There was a problem hiding this comment.
It's like a proposal shape but for terms.
Documentation is on the TODO list:
That issue has pointers to context.
contract/src/offer-up.contract.js
Outdated
| exit: M.any(), | ||
| }); | ||
|
|
||
| /** a seat for allocating proceeds of sales */ | ||
| const proceeds = zcf.makeEmptySeatKit().zcfSeat; | ||
|
|
||
| /** @param {ZCFSeat} buyerSeat */ |
There was a problem hiding this comment.
| /** @param {ZCFSeat} buyerSeat */ | |
| /** @type {OfferHandler} */ |
This maybe gives more info to the user - they can learn about the second optional argument offerArgs
There was a problem hiding this comment.
It seems like an extra level of indirection to explain in the zoe walkthru.
If we had good auto-generated reference docs for OfferHandler, then yeah... but we don't, so...
| * @param {ZCF<{tradePrice: Amount}>} zcf | ||
| * This contract is parameterized by terms for price and, | ||
| * optionally, a maximum number of items sold for that price (default: 3). | ||
| * |
There was a problem hiding this comment.
| * | |
| * These terms will extend `StandardTerms` present in all zoe contracts, `issuers` and `brands`. | |
| * |
Don't feel strongly about this suggestion but could be helpful.
Hard to have an opinion without seeing what the revised docs look like. I would imagine 1-2 line comments are more tenable in a docs site versus multi-line. |
contract/src/offer-up.contract.js
Outdated
| * - handles offers to buy up to `maxItems` items at a time. | ||
| * | ||
| * As is typical in Zoe contracts, the flow is: | ||
| * 1. contract does internal setup |
There was a problem hiding this comment.
| * 1. contract does internal setup | |
| * 1. contract does internal setup (mint assets) |
Can we expand on 'internal setup'?
There was a problem hiding this comment.
Well, it doesn't mint any assets at startup. It creates a mint, but doesn't use it until an offer is made.
The point of this comment was to clarify the order that readers should use to look at the code. They can see the rest for themselves, I would think.
In fact, now that I think about it, this part of the comment doesn't belong on the doctring of start; it's not aimed at clients of start. It's an internal comment aimed at readers of the implementation. I'm moving it to the file comment.
- add comments to clarify the control flow etc., - note our `no-use-before-define` convention (in prose) - cite zoe overview walkthru of this code - cite Hardened JS docs - feat: hoist maxItems to default from terms instead of hard-coded - use atomicRearrange() instead of deprecated reallocate() rather than try to explain - validate contract terms with patterns - refactor bagValueSize() into sum() and bagCounts() - set off with "#region bag utilities" - punt trace() rather than explain
dckc
force-pushed
the
dc-contract-docs2
branch
from
ec8357f to
d58a631
Compare
@kbennett2000 made a proposal a while back to add comments; he and I refined that into #38 . I have since split out the renaming part as #45 . A question for @samsiegart @0xpatrickdev @turadg @Chris-Hibbert etc. remains: how to handle the overlap between the comments here and the walkthru prose in the contract code walk-thru in the zoe docs? Copy the comments here into the walkthru code snippets? I suppose I'll have to try it out to see what looks most natural.
@kbennett2000 rather than changing the contract surface API again (and updating the UI again), this doesn't rename
PricetoPayetc. as we did in #38What it does:
no-use-before-defineconvention (in prose)