Make Recovery sets optional #9013
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Chris-Hibbert
added
performance
14 tasks
Chris-Hibbert
force-pushed
the
8400-recoverySets-nonIncremental
branch
from
217a3cd to
0a87b88
Compare
|
#8740 has more recent theorizing. |
erights
approved these changes
Mar 1, 2024
Comment on lines
+179
to
+180
| optRecoverySet === undefined || | ||
| Fail`when recoverSetsState === 'noRecoverySets', optRecoverySet must be empty`; |
| optRecoverySet === undefined || | ||
| Fail`when recoverSetsState === 'noRecoverySets', optRecoverySet must be empty`; | ||
| } | ||
| if (optRecoverySet !== undefined && !AmountMath.isEmpty(amount)) { |
There was a problem hiding this comment.
Good to see the isEmpty check arrive too!
packages/ERTP/src/types-ambient.js
Outdated
Comment on lines
230
to
232
| * 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.
Please also state the restriction in the opposite direction, using the same "At this time..." wording.
Chris-Hibbert
added
resource-exhaustion
Chris-Hibbert
force-pushed
the
8400-recoverySets-nonIncremental
branch
from
d4fdd43 to
aa4b7db
Compare
This was referenced May 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.