Skip to content
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

Vaults detects failing priceAuthority and requests new one #8696

Merged
merged 5 commits into from
Jan 6, 2024

Conversation

Chris-Hibbert
Copy link
Contributor

refs: #8675

Description

Define watchPerpetualNotifier, and use it to monitor updates from the quoteNotifier. When it detects an error, it requests a new notifier and restarts the watchPerpetualNotifier.

Security Considerations

robustness for the chain.

Scaling Considerations

Not a scaling issue.

Documentation Considerations

No user docs required.

Testing Considerations

Added a test that Vaults switches to a new priceAuthority when the existing authority throws an error.

Many existing vaults tests were not cleanly adding a priceAuthority before adding a new vaultManager, so I had to refactor the setup functions to make that simple.

Upgrade Considerations

The priceAuthority is holding onto cyclic data structures involving exitObjects. In order to force GC of those objects, we plan to fix the problem in Zoe and then abandon the old priceAuthorities. This fix allows Vaults to survive when the old priceAuthority is killed.

@Chris-Hibbert Chris-Hibbert self-assigned this Dec 28, 2023
@Chris-Hibbert Chris-Hibbert added performance Performance related issues resource-exhaustion Threats to availability from resource exhaustion attacks oracle Related to on-chain oracles. Vaults VaultFactor (née Treasury) labels Dec 28, 2023
priceAuthority,
priceAuthority: priceAuthorityReg,
priceAuthorityAdmin,
aethPriceAuthority,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why expose this? anything looking up aeth prices should be able to get it from priceAuthority

OIC, it's to provide access to the manual authority. consider naming it aethMPA or something to distinguish.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider it a bug farm that the thing called priceAuthority rather pervasively, is actually a priceAuthorityRegistry with the same API as a priceAuthority. I don't mind giving the manual PA a distinct name, since one has to know its type to be able to use its manual properties.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider it a bug farm that the thing called priceAuthority rather pervasively, is actually a priceAuthorityRegistry with the same API as a priceAuthority

I don't think that's quite right. I refer to this type:

/**
* @typedef {object} PriceAuthorityRegistry A price authority that is a facade
* for other backing price authorities registered for a given asset and price
* brand
* @property {PriceAuthority} priceAuthority
* @property {PriceAuthorityRegistryAdmin} adminFacet
*/

PriceAuthority is an interface. PriceAuthorityRegistry is a kit of a priceAuthority (PriceAuthority) and an adminFacet (PriceAuthorityRegistryAdmin) which can be used to control it.

Regardless of the provenance, the PriceAuthority interface allows for querying any price, whether the authority has information about it or not.

Do you want to change that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the shared interface that has my attention. I'm a strong proponent of polymorphism.

What I'm referring to is the confusion of the priceAuthorityRegistry with other priceAuthorities. I've only gradually (over the last two quasi-vacation weeks) come to understand how all these pieces actually fit together.

  • The priceAuthorityRegistry is a vat that pre-dates Zoe in bootstrap order. (It's Vat8, while Zoe is Vat9). That means that it's not a Zoe contract. I don't know what that means for our ability to upgrade it or inform its dependents if it changes.
  • When provided to Auctions and Vaults, its name is PriceAuthority and it is typed as PriceAuthority. I had assumed that I was using a PriceAuthority in those contracts.

If you have a PriceAuthorityRegistry, and you call makeQuoteNotifier(), you end up talking to an ERef for the notifier that refers directly to the intended priceAuthority vat. (This is fine.) If you use any of the other PA methods for requesting a price, every request is relayed through the PriceAuthorityRegistry vat.

ln the context of tracing an associated storage leak (#8400), knowing which vats are holding and relaying which promises has become a priority.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When provided to Auctions and Vaults, its name is PriceAuthority and it is typed as PriceAuthority. I had assumed that I was using a PriceAuthority in those contracts.

Isn't that accurate? It may leave out some details but it is an object that implements PriceAuthority. That's all the Auctions and Vaults are supposed to know.

I'm interested in what changes you might propose. But we're on a tangent now so I'm fine resolving this thread and picking up in Slack or wherever.

packages/notifier/src/asyncIterableAdaptor.js Outdated Show resolved Hide resolved
space,
priceAuthority: priceAuthorityReg,
priceAuthorityAdmin,
aethPriceAuthority,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same issue here, right?

Suggested change
aethPriceAuthority,
aethManualPA,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Chris-Hibbert Chris-Hibbert added the automerge:squash Automatically squash merge label Jan 6, 2024
@Chris-Hibbert Chris-Hibbert merged commit 4a6d7f4 into 8675-auction-newPA Jan 6, 2024
70 of 71 checks passed
@Chris-Hibbert Chris-Hibbert deleted the 8675-vaults-newPA branch January 6, 2024 00:44
Chris-Hibbert added a commit that referenced this pull request Jan 8, 2024
If the quoteNotifier is broken, set updatingOracleQuote to null. If
updatingOracleQuote is null when attempting to capture the oracle
price, restart observeQuoteNotifier

Vaults detects failing priceAuthority and requests new one (#8696)
mergify bot added a commit that referenced this pull request Jan 9, 2024
)

* feat: auctioneer/vaults detect failing priceAuthority & request new

If the quoteNotifier is broken, set updatingOracleQuote to null. If
updatingOracleQuote is null when attempting to capture the oracle
price, restart observeQuoteNotifier

Vaults detects failing priceAuthority and requests new one (#8696)

* feat: make vaultManager robust against failing priceAuthority

* chore: fix types, drop expect-error

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@Chris-Hibbert Chris-Hibbert mentioned this pull request Feb 7, 2024
14 tasks
anilhelvaci pushed a commit to Jorge-Lopes/agoric-sdk that referenced this pull request Feb 16, 2024
…oric#8691)

* feat: auctioneer/vaults detect failing priceAuthority & request new

If the quoteNotifier is broken, set updatingOracleQuote to null. If
updatingOracleQuote is null when attempting to capture the oracle
price, restart observeQuoteNotifier

Vaults detects failing priceAuthority and requests new one (Agoric#8696)

* feat: make vaultManager robust against failing priceAuthority

* chore: fix types, drop expect-error

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge oracle Related to on-chain oracles. performance Performance related issues resource-exhaustion Threats to availability from resource exhaustion attacks Vaults VaultFactor (née Treasury)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants