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

refactor smartWallet error handling on executeOffer #8997

Closed
Chris-Hibbert opened this issue Feb 26, 2024 · 2 comments · Fixed by #8998
Closed

refactor smartWallet error handling on executeOffer #8997

Chris-Hibbert opened this issue Feb 26, 2024 · 2 comments · Fixed by #8998
Assignees
Labels
enhancement New feature or request wallet

Comments

@Chris-Hibbert
Copy link
Contributor

What is the Problem Being Solved?

In #8488, SmartWallet was refactored to use promiseWatcher to ensure that promises survived various kinds of upgrade trauma.

That refactoring left some error handling in a catch clause in executeOffer, missing the fact that that error handling would be lost when the walletFactory itself is upgraded.

Description of the Design

Move the contents of the catch clause in executOffer to appropriate sections of offerWatcher.js.

Security Considerations

Ensure that wallets correctly report on exceptions.

Scaling Considerations

Not much relevance.

Test Plan

It may be difficult to provoke the error conditions (multiple upgrades, and exceptions at carefully calibrated times would be required). We aren't going to try to write tests for these corner cases, and will instead review carefully.

Upgrade Considerations

This is about error behavior in upgrade situations. With the refactor, we expect the error handling to be invoked correctly even if there are vat upgrades.

@dckc
Copy link
Member

dckc commented Apr 10, 2024

"refactor"? this is a bug, no?

@turadg
Copy link
Member

turadg commented Apr 10, 2024

"refactor"? this is a bug, no?

fwiw, the commit message was fix: 5397e0c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wallet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants