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

fix(wallet): handle cancelled offer state #4292

Merged
merged 3 commits into from
Jan 21, 2022
Merged

fix(wallet): handle cancelled offer state #4292

merged 3 commits into from
Jan 21, 2022

Conversation

samsiegart
Copy link
Contributor

Fixes: #4243
Screenshot:
image

@samsiegart
Copy link
Contributor Author

I wonder if it makes sense to show what assets were actually transferred vs returned inside the "completed" offer. I noticed that even if you were outbid on a card the offer shows "completed" but you didn't actually lose your give/get your want.

@michaelfig
Copy link
Member

I wonder if it makes sense to show what assets were actually transferred vs returned inside the "completed" offer. I noticed that even if you were outbid on a card the offer shows "completed" but you didn't actually lose your give/get your want.

This is a good idea, maybe a "refunded" state. Zoe currently doesn't do anything to distinguish this result, so it would take some work on the Zoe service to be able to report it. We could instead hack together something in the wallet backend to heuristically determine that it was a refund (not my preference).

@erights, thoughts?

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM! Just one suggestion to make a conditional terser.

Comment on lines 199 to 205
const isOfferCompleted =
status === 'accept' ||
status === 'decline' ||
status === 'complete' ||
status === 'rejected' ||
status === 'cancel';

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const isOfferCompleted =
status === 'accept' ||
status === 'decline' ||
status === 'complete' ||
status === 'rejected' ||
status === 'cancel';
const isOfferCompleted = ['accept', 'decline', 'complete', 'rejected', 'cancel'].includes(status);

Copy link
Member

@erights erights Jan 13, 2022

Choose a reason for hiding this comment

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

@samsiegart @michaelfig
Where does this status string come from? Where are its possible values defined?

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'm not sure if we have any good documentation on that. The implementation is in https://github.com/Agoric/agoric-sdk/blob/master/packages/wallet/api/src/lib-wallet.js (search for "status:")

@samsiegart
Copy link
Contributor Author

Will merge to fix #4243, we can continue the discussion here or open an issue if need be

@samsiegart samsiegart added the automerge:squash Automatically squash merge label Jan 21, 2022
@mergify mergify bot merged commit 9dd2dc4 into master Jan 21, 2022
@mergify mergify bot deleted the wallet-exit branch January 21, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wallet UI: "Cancelled" offers show buggy state
3 participants