-
Notifications
You must be signed in to change notification settings - Fork 205
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
feat: add exit to empty seat #1584
Conversation
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 looks like it continues to conserve all our safety properties, but I don't understand its value.
Under what circumstances would a contract want to create an internal seat for which it needed an exit clause?
Many circumstances - for instance, if the seat should exit after a particular deadline. Or, if the userSeat facet will be handed to someone else such that the ability to exit is separated from the contract's ability to exit. My point, though, is that the burden of proof has been in the wrong direction. We shouldn't be asking why seats should all act the same. We should be asking why some seats should have different functionality. I would like the empty seats to have the same functionality as other seats. |
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 like the switching burden of proof argument. But it seems this PR is only a step along the way. The result is far from like a seat in all ways.
A seat whose want
alone must be satisfied to preserve offer safety would take it farther from being just like a normal seat. I also don't see how it is a workable plan. The seat would start with an allocation that VIOLATES its offer safety. If the contract crashes in this state, offer safety would be violated. If the seat is exited in this state, what happens? Preventing the exit violates payout liveness.
So putting aside the burden-of-proof argument, it would help to actually have and try a concrete use case. The issues under even an artificial but concrete use case may be much easier to evaluate than under imagined use.
Was there a particular use case you had in mind that motivated this in the first place?
I think I disagree with this. The only way it is different is the offer cannot I view this PR as a step towards being able to
Yup, that makes sense.
Yes, 1) the ability to exit an empty seat automatically I think I will try to change this PR to allow the contract code to pass only the exit rule, so that the |
By "updated" do you mean modified after creation? I don't think so, and your current code only provides proposal ingredients when the seat is constructed. This makes sense to me. With that clarified, I look forward to the next version. Thanks. |
No, sorry, updated is the wrong word. I mean created. |
2c01a68
to
84bc8b1
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.
LGTM
packages/zoe/src/types.js
Outdated
@@ -208,7 +208,7 @@ | |||
* @property {(brand: Brand) => Issuer} getIssuerForBrand | |||
* @property {GetAmountMath} getAmountMath | |||
* @property {MakeZCFMint} makeZCFMint | |||
* @property {() => ZcfSeatKit} makeEmptySeatKit | |||
* @property {(want: AmountKeywordRecord=, exit: ExitRule= }) => ZcfSeatKit} makeEmptySeatKit |
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.
Stale want:
. Please remove.
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.
Good point
offerResult: offerResultPromiseKit.promise, | ||
exitObj: exitObjPromiseKit.promise, | ||
}, | ||
exitObjPromiseKit.promise, |
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.
Didn't you already flatten this out in a previous PR? In any case, the flattening is good.
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.
Whoops, I've got some rebasing to do, sorry.
84bc8b1
to
b49d59f
Compare
See #1582 for more discussion. I'm curious if we could implement the
give
portion too, using a reallocation.I still need to add tests of the different userSeat methods to ensure the empty seat operates like a real seat in all ways.
Closes #1582