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

smart wallet may drop offer notification (e.g. bids) on upgrade #8286

Closed
dckc opened this issue Aug 30, 2023 · 7 comments
Closed

smart wallet may drop offer notification (e.g. bids) on upgrade #8286

dckc opened this issue Aug 30, 2023 · 7 comments
Assignees
Labels
bug Something isn't working contract-upgrade triage_23_10 DO NOT USE wallet

Comments

@dckc
Copy link
Member

dckc commented Aug 30, 2023

Describe the bug

While investigating #8245, we found outstanding offers waiting for payouts. If Zoe is upgraded, the promise for the payouts will reject. The smartWallet code keeps its reference to the relevant seat, so the user can exit the offer. But it's not clear that they'll be notified promptly if the contract pays out.

Similarly, if the walletFactory is upgraded, the code processing the offer will not resume automatically.

To Reproduce

The bug is hypothesized; it has not yet been observed

  1. Make a bid
  2. Upgrade zoe
  3. win auction
  4. no change in wallet vstorage state

Expected behavior

Notifications of winning bids should still work after upgrading zoe.

Long-standing offers such as liquidation bids should survive upgrade of zoe.

Platform Environment

  • what version of the Agoric-SDK are you using? (run git describe --tags --always)

mainnet1B

  • what OS are you using? what version of Node.js?
  • is there anything special/unusual about your platform?

follower node

Additional context

The promise @warner and I looked at in particular is:

subscriber decider kpid
v43-walletFactory v9-zoe kp356888

The suspect code is:

https://github.com/Agoric/agoric-sdk/blob/mainnet1B/packages/smart-wallet/src/offers.js#L142-L152

https://github.com/Agoric/agoric-sdk/blob/mainnet1B/packages/zoe/src/zoeService/zoeSeat.js#L318-L325

@dckc dckc added bug Something isn't working wallet labels Aug 30, 2023
@dckc
Copy link
Member Author

dckc commented Aug 30, 2023

false alarm: the seat is kept, giving the user the ability to exit. They might not be notified promptly of winning an auction after zoe is upgraded, though.

https://github.com/Agoric/agoric-sdk/blob/mainnet1B/packages/smart-wallet/src/smartWallet.js#L503

@dckc dckc changed the title smart wallet may lose seats, assets if zoe is upgraded smart wallet may not notify promptly if zoe is upgraded Aug 30, 2023
@dckc
Copy link
Member Author

dckc commented Aug 31, 2023

@erights @Chris-Hibbert how should a zoe client handle a broken promise from E(seat).getPayouts()? Retrying is an option, but it seems to invite infinite loops.

@erights
Copy link
Member

erights commented Aug 31, 2023

See how @gibson042 handles such retries in notifier/subscriber to avoid infinite loops. @gibson042 what's the best thing to look at first?

Of all the outstanding remote promises, the promises for payouts is the most important severing to recover from. Fortunately, you can just ask the seat again for the payouts, given that we can adapt @gibson042 's retry techniques to this case.

@gibson042
Copy link
Member

gibson042 commented Aug 31, 2023

reconnectAsNeeded uses isUpgradeDisconnection to detect situations where recovery from a broken promise may be possible by calling out to a provided getter function that is itself capable of returning similarly broken promises, and tolerates an arbitrarily long sequence of those as long as the incarnationNumber of each is strictly increasing.

@erights
Copy link
Member

erights commented Aug 31, 2023

Yes, that's exactly what I had in mind. Thanks!

@dckc
Copy link
Member Author

dckc commented Aug 31, 2023

ah... const { incarnationNumber: version } = err;

So we take the other end's word for it regarding incarnationNumber.

@dckc dckc changed the title smart wallet may not notify promptly if zoe is upgraded smart wallet may stop offer notification on upgrade Aug 31, 2023
@dckc dckc changed the title smart wallet may stop offer notification on upgrade smart wallet may drop offer notification (e.g. bids) on upgrade Sep 1, 2023
@aj-agoric aj-agoric added this to the Upgrade 12 code complete milestone Sep 13, 2023
@Chris-Hibbert
Copy link
Contributor

Fixed by #8445.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working contract-upgrade triage_23_10 DO NOT USE wallet
Projects
None yet
Development

No branches or pull requests

5 participants