some JSDoc updates for Zoe #9001
Conversation
| @@ -159,7 +160,7 @@ export const makeZoeSeatAdminFactory = baggage => { | |||
| assertHasNotExited(this, 'Cannot exit seat. Seat has already exited'); | |||
|
|
|||
| state.exiting = true; | |||
| E.when( | |||
| void E.when( | |||
There was a problem hiding this comment.
As discussed, this solves the problem with the linter, but leaves error messages in production logs that are being noticed. I think we should take care of both with a noop catch clause.
There was a problem hiding this comment.
@mhofman you tend to be most affected by unhandled promise rejections in the logs. Do you prefer a rejection here to be silent?
There was a problem hiding this comment.
I don't think I have enough background on the behavior. what are the consequences of doExit failing? It seems to not be reported anywhere except in console output. I thought voiding a promise was meant to say "I've handled the rejection, trust me".
There was a problem hiding this comment.
I thought voiding a promise was meant to say "I've handled the rejection, trust me".
Close. It's saying "I'm intentionally not handling this rejection". It tells the linter (and the reader) that the absence of handling is intentional.
what are the consequences of doExit failing?
@Chris-Hibbert ? Is it a "this could happen and we don't care" or "this should never happen so if it does we should find out" ?
There was a problem hiding this comment.
I do not understand what the complaints in the log are unhappy about. This is already in an E.when() with an onRejected branch.
If the E.when() itself fails, and the onRejected clauses don't do their job, aren't we in a world of hurt?
@turadg and I talked about this out-of-band, and we concluded that E.when() may need a void in order to make the linter happy, but a noop catch clause would never prevent messages to the logs. In the future, I'll ask for noop catches only on other kinds of bare promises.
packages/zoe/src/internal-types.js
Outdated
| @@ -59,6 +59,8 @@ | |||
| * @property {ShutdownWithFailure} fail called with the reason | |||
| * for calling fail on this seat, where reason is normally an instanceof Error. | |||
| * @property {() => Subscriber<AmountKeywordRecord>} getExitSubscriber | |||
| * @property {(result: HandleOfferResult) => void} resolveExitAndResult | |||
| * @property {(paymennts: PaymentPKeywordRecord) => Promise<void>} finalPayouts | |||
There was a problem hiding this comment.
| * @property {(paymennts: PaymentPKeywordRecord) => Promise<void>} finalPayouts | |
| * @property {(payments: PaymentPKeywordRecord) => Promise<void>} finalPayouts |
packages/zoe/src/zoeService/types.js
Outdated
| * seat has exited. | ||
| * @property {() => Subscriber<Completion>} getExitSubscriber returns a subscriber that | ||
| * will be notified when the seat has exited or failed. | ||
| * @property {() => void} [tryExit] Note: Only works if the seat's `proposal` |
There was a problem hiding this comment.
why [tryExit]? The function is there regardless, it'll just throw if the exit rule isn't right. The throw will be detectably from inside Zoe and not from the guards.
There was a problem hiding this comment.
I didn't change the optionality, just word wrapping. But is is wrong so I'll correct it.
packages/zoe/src/zoeService/types.js
Outdated
| * contract might do something explicitly. On exiting, the seat holder gets its | ||
| * current `allocation` and the `seat` can no longer interact with the contract. | ||
| * @property {() => Promise<boolean>} hasExited Returns true if the seat has | ||
| * exited, false if it is still active. |
There was a problem hiding this comment.
| * exited, false if it is still active. | |
| * exited, false if it is still active. Returns promptly. |
| * promise for the Payment corresponding to the indicated keyword. The promise | ||
| * will resolve after the seat has exited. |
There was a problem hiding this comment.
Let's use consistent terminology about the timing of the response.
| * promise for the Payment corresponding to the indicated keyword. The promise | |
| * will resolve after the seat has exited. | |
| * promise for the Payment corresponding to the indicated keyword. The promise | |
| * will be resolved promptly once the seat exits. |
packages/zoe/src/zoeService/types.js
Outdated
| * | ||
| * @property {(keyword: Keyword) => Promise<Payment<any>>} getPayout returns a | ||
| * promise for the Payment corresponding to the indicated keyword. The promise | ||
| * will resolve after the seat has exited. | ||
| * @property {() => Promise<OR>} getOfferResult |
There was a problem hiding this comment.
Please don't leave this uniquely undocumented. Feel free to reword. The line about timing is crucial for parallelism with the other comments.
Returns a Promise for an OfferResult. The OfferResult can be any Passable. For example, in the Automatic Refund example, it's the string "The offer was accepted". In the Covered Call example, it's a call option, which is an assayable Invitation to buy the underlying asset. Strings and invitations are the most common things returned. The value is the result returned by the offerHandler function passed in the first argument to zcf.makeInvitation(...).
Since the contract can return whatever it wants as an offer result, there is no guarantee that the promise will resolve promptly.
turadg
added
automerge:rebase
|
This got stale since February and now it 1) has conflicts with master and 2) reveals type system failures with Exos. I'm dropping it to draft until I find time to fix the type failures. @dckc feel free to cherry-pick anything useful to you |
experiment to see how it will look in JSdoc. Markdown tables don't appear to be possible
| * put in. If the contract attempts to satisfy it, they either get what they | ||
| * asked for or Zoe ensures they get back their deposit. | ||
| * | ||
| * Example: |
There was a problem hiding this comment.
| * Example: | |
| @example |
cf @example tag
refs: #4291
Description
A little progress on #4291 to see the effort required. It was mostly copy-paste. JSDoc, at least in VS Code rendering, doesn't support Markdown tables so I changed one to a bullet list.
There is much more to do for 4291 but I think this is worth landing. It also annotates the types on the implementation so that hover brings along the JSDoc.
Security Considerations
n/a
Scaling Considerations
n/a
Documentation Considerations
Eventually build the API parts of docs site from the API source code.
Testing Considerations
n/a
Upgrade Considerations
n/a