-
Notifications
You must be signed in to change notification settings - Fork 231
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(ERTP): Make recoverySets optional #8406
Conversation
38c47ae
to
5519cdb
Compare
// NOTE: Schema change! | ||
recoverySet: M.opt(M.remotable('recoverySet')), |
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.
Reviewers,
NOTE: Schema change!
What should we actually do about this?
const paymentRecoverySets = | ||
recoverySetOption === 'noRecoverySets' | ||
? undefined | ||
: provideDurableWeakMapStore(issuerBaggage, 'paymentRecoverySets'); | ||
|
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.
What if the revived issuer has 'noRecoverySets'
but the old state has recovery sets?
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 all looks quite plausible to me, but I don't think I'm qualified to comment on whether it's good to go, so I'll leave the approving part to somebody who knows this stuff better.
5519cdb
to
2d5c300
Compare
2d5c300
to
cafb065
Compare
9eb3636
to
ead568a
Compare
ead568a
to
dc6e0f6
Compare
Closing in favor of #8418 |
TODO explain
Staged on #8414