-
Notifications
You must be signed in to change notification settings - Fork 208
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
Make Recovery sets optional #9013
Conversation
217a3cd
to
0a87b88
Compare
#8740 has more recent theorizing. |
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! Thanks for staying close to code I already understood.
optRecoverySet === undefined || | ||
Fail`when recoverSetsState === 'noRecoverySets', optRecoverySet must be empty`; |
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.
Thx for expanding this out.
optRecoverySet === undefined || | ||
Fail`when recoverSetsState === 'noRecoverySets', optRecoverySet must be empty`; | ||
} | ||
if (optRecoverySet !== undefined && !AmountMath.isEmpty(amount)) { |
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 to see the isEmpty check arrive too!
packages/ERTP/src/types-ambient.js
Outdated
* At this time, a `'noRecoverySets'` predecessor cannot be upgraded to a | ||
* `'hasRecoverySets'` successor. If it turns out this transition is needed, it | ||
* can likely be supported in a future upgrade. |
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.
Please also state the restriction in the opposite direction, using the same "At this time..." wording.
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.
done.
d4fdd43
to
aa4b7db
Compare
refs: #8400
closes: #8498
closes: #8418
refs: #8928
Description
This change makes it possible, when creating an issuer, to decline to use recover sets. It also converts priceAuthority and fluxAggregator to do so. This only affects new instances of these contracts. The existing instances hold large numbers of old no-longer-used quote-payments which are currently uncollectible.
This PR was based on #8498 (itself based on #8418) which would have gradually removed quote-payments from the recoverySet. We aren't taking that approach because those vats are not upgradeable. #8418 would have released all the unneeded quote-payments at once, which would likely have overwhelmed the GC system.
#8400 talks about the general problem. #8928 talks about one possible long-term fix cleaning up the retained garbage.
Security Considerations
N/A
Scaling Considerations
It's all about reducing the impact of large quantities of unneeded quotes. This PR will stop us from accumulating more quotes.
Documentation Considerations
N/A
Testing Considerations
We will validate in a chain environment.
Upgrade Considerations
This introduces new code for the priceAuthority and fluxAggregator contracts, which will be used to create new instances which will be more performant. The existing vats will be addressed later.