-
Notifications
You must be signed in to change notification settings - Fork 214
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
vat-provisionpool will not re-subscribe to vat-bank getCurrentAmountNotifier #8722
Labels
Comments
mergify bot
added a commit
that referenced
this issue
Jun 12, 2024
fixes: #8722 fixes: #8724 fixes: #8238 ## Description Upgrade provisionPool so its notifiers are robust to upgrades. ### Security Considerations This is about soundness of the chain and its services. ### Scaling Considerations N/A ### Documentation Considerations No change to functionality. ### Testing Considerations Includes a test in `a3p-integration` that demonstrates that provisionPool will survive its own or vat-bank's upgrade. ### Upgrade Considerations See above.
mergify bot
added a commit
that referenced
this issue
Dec 3, 2024
…grade (#10419) closes: #10395 closes: #10425 closes: #10564 ## Description As mentioned in #8158, we need the ability to restart or replace all vats. This PR focuses on the vats v28-provisionPool and v14-bank. The goal is to make sure after upgrading both of those vats, v28-provisionPool functionality keeps working. We pay special attention to #8722 and #8724 during the tests as those issues were identified as potential problems against v28-provisionPool upgrade. ### Security Considerations v28-provisionPool and v14-bank are critical vats for the system when it comes to onboarding new users and keeping track of ERTP representations of user assets. Reviewers, please highlight the slightest risk you see. ### Scaling Considerations v28-provisionPool vat has a linear relationship with the number of users entering the Agoric system. So it is pretty important it keeps working. Reviewers, please highlight the slightest risk you see. ### Documentation Considerations None. ### Testing Considerations During the testing, we focused on `provisionPoolAddress`' cosmos level balances as our source of truth. The reasoning behind this is; if the IST balance of the `provisionPoolAddress` it can only mean - it has received the anchor asset we've sent - v28-provisionPool is notified of this balance change - executed a swap against corresponding PSM - deposited the payout to IST purse - v14-bank updated the balances correctly In our test we send two anchor assets; * USDC_axl to make sure v28-provisionPool recovered existing purses * USD_LEMONS to make sure v28-provisionPool is notified of new assets ### Upgrade Considerations Both v28-provisionPool and v14-bank are staged upgrades in `upgrade.go`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
As part of #8499 (comment) (look for
v14-bank-v28-provisionpool
), we identified a likely problem in the ProvisionPool vat. It has about a dozen promises that are subscribed to v14-bank, which will probably not re-subscribe when v14-bank is restarted/upgraded. As such, it represents an upgrade blocker: we must deploy a fix to v28-provisionpool before we can safely upgrade v14-bank.v28-provisionpool uses
observeNotifier
to subscribe to a notifier obtained withE(exchangePurse).getCurrentAmountNotifier()
. The use ofobserveNotifier
is great, it knows how to react to vat upgrades, however the notifier it is pulling from (created by ERTP) is not durable. SoobserveNotifier
will see an unrecoverable error, and will stop following.We need the provisionpool code to react to this by calling
getCurrentAmountNotifier()
again (perhaps thefail:
clause can inspect thereason
and, if it is a new disconnection, try again).Upgrade Considerations
v28-provisionpool does not use durable followers, so if/when v28 is upgraded, all of the
observeNotifier
instances will go away. This presents a problem when upgrading v28. We need the provisionpool code to somehow re-acquire all the asset subscriptions at upgrade time. This needs to be present in the next deployment of v28, to avoid losing track of the amounts that it is supposed to be watching. This yields the following sequence of upgrade dependencies:The first two v28 deployments can be merged.
The text was updated successfully, but these errors were encountered: