-
Notifications
You must be signed in to change notification settings - Fork 212
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
refactor: Vaults get price updates continuously #7351
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.
A good improvement but requesting we resolve observeNotifier
problems before landing
@@ -23,7 +23,7 @@ const { quote: q, Fail } = assert; | |||
|
|||
const { StorageNodeShape } = makeTypeGuards(M); | |||
|
|||
const trace = makeTracer('IV', false); | |||
const trace = makeTracer('Vault', false); |
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.
fwiw this was for InnerVault but this change lgtm
E(storageNode).makeChildNode('quotes'), | ||
marshaller, | ||
); | ||
let storedCollateralQuote; | ||
observeNotifier(quoteNotifier, { |
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 observeNotifier
will fail upgrade. #6791 the ticket to make this contract upgradable, so this PR doesn't have to be upgradable but we should at least have a sketch of how this will be upgraded.
Also it will consume heap so I want to hear from @mhofman on whether landing this would complicate his research.
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.
Would this result a per-vault heap object? Or does this happen only after some trigger for a vault?
We should definitely hunt down any patterns that result in high cardinality heap objects.
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.
Per-manager heap object. There are order 10 managers.
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.
Sounds like we don't need to worry about the heap usage. The upgrade aspect I'll taclke in #6791.
If you do another push, consider moving this observeNotifier
call to finish()
with the other one.
E(storageNode).makeChildNode('quotes'), | ||
marshaller, | ||
); | ||
let storedCollateralQuote; | ||
observeNotifier(quoteNotifier, { |
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.
Sounds like we don't need to worry about the heap usage. The upgrade aspect I'll taclke in #6791.
If you do another push, consider moving this observeNotifier
call to finish()
with the other one.
closes: #6946 VaultManagers were already channeling a subscription to the priceAuthority in order to store it off chain. This uses that feed instead of requesting a fresh price when needed.
8ddbee4
to
df6575b
Compare
closes: #6946
Description
VaultManagers were already channeling a subscription to the priceAuthority in order to store it off chain. This uses that feed instead of requesting a fresh price when needed.
Security Considerations
None
Scaling Considerations
slight improvement.f
Documentation Considerations
No impact on docs.
Testing Considerations
updated existing tests.