-
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
vat-walletFactory upgrade would cause wallet to stop tracking existing offers #8445
Labels
Comments
Chris-Hibbert
added a commit
that referenced
this issue
Oct 26, 2023
Chris-Hibbert
added a commit
that referenced
this issue
Oct 26, 2023
Chris-Hibbert
added a commit
that referenced
this issue
Oct 26, 2023
Chris-Hibbert
added a commit
that referenced
this issue
Oct 27, 2023
mergify bot
added a commit
that referenced
this issue
Oct 27, 2023
test: show that smartWallet fails if zoe upgrades. due to #8445
iomekam
pushed a commit
that referenced
this issue
Oct 27, 2023
Merged
This was referenced Jan 10, 2024
This was referenced Jan 18, 2024
anilhelvaci
pushed a commit
to Jorge-Lopes/agoric-sdk
that referenced
this issue
Feb 16, 2024
mergify bot
added a commit
that referenced
this issue
Oct 23, 2024
closes: #9404 ## Description In #8445, we repaired legacy smartWallets. This removes the code that ran in upgrade15 that did the cleanup. ### Security Considerations None ### Scaling Considerations None ### Documentation Considerations None ### Testing Considerations No tests should be removed. ### Upgrade Considerations There is no hurry to merge this code on chain. If the PR merges soon, it'll be included in upgrade 18.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
#8286 describes some problems that vat-walletFactory will have upon upgrade, but merges two separate problems. I'm leaving #8286 to capture what happens to vat-walletFactory when vat-zoe is upgraded, and this new ticket captures what happens to vat-walletFactory when vat-walletFactory itself is upgraded:
The code in vat-walletFactory which subscribes to
numWantsSatisfied()
andgetPayouts()
andgetOfferResult()
(https://github.com/Agoric/agoric-sdk/blob/d4b32f129868ed2faa32f1bcad51b5d2248d4c14/packages/smart-wallet/src/offers.js) is run in a wallet function namedmakeOfferExecutor
, and used by theexecuteOffer()
method. As best I can tell, this is the only time these three subscriptions occurs.So if an offer is opened in vat-walletFactory incarnation-1, then vat-walletFactory is upgraded to incarnation-2, the new incarnation will not be following any of those promises. The seat might still be active, or it might exit some time after the upgrade, but I don't see any code in vat-walletFactory that would notice.
To resolve this, the wallet needs to either:
Fix 1 will require O(N) time spent shortly after upgrade, as it needs to iterate through all open seats. Fix 2 avoids O(N) work.
To remediate the problem for existing open seats, we need 1. To avoid making the problem worse, we need 2 as well.
Deployment Considerations
We need this fix to be included in the next mainnet vat-walletFactory deployment: we cannot safely upgrade vat-walletFactory without it.
The text was updated successfully, but these errors were encountered: