Misleading status message when AMM offer is rejected

[turadg]

Describe the bug

If the AMM offer is processed it returns "Swap successfully completed." even if the offer was rejected. For example, due to making an offer that's not on the AMM curve.

Expected behavior

Some response that informs the user of the new state.

Additional context

The REPL is part of the UI, though not GUI.

See also #3056

[Tartuffo]

Need to properly surface the outcome of the offer.
Should it throw or politely return the "give"?
Can handle entirely in the wallet, but will be seen in the REPL.

[dckc]

another awkward consequence of not throwing is a notifier update with no change in state: https://github.com/Agoric/agoric-sdk/pull/5420/files#r879991457

[turadg]

Either this or #3056 needs to be done for MN-1.

Need to discuss with @erights which. When determined then update both tickets.

[erights]

Yes, let's discuss. @dtribble and I discussed a method that could easily be added to UserSeat that would help. But I am not sure I understand the problem and why the wallet could not easily do the equivalent of that method for itself.

I am not that oriented in the wallet, so I may be missing something obvious. It seems the problem is that the wallet is paying looking at the offerResult when it should be looking at the payout. The question it seems to want to ask is "Is this offer's proposal's want satisfied?" The UserSeat method @dtribble and I discussed, that @dtribble thinks would help, is an isSatisfied method that returns a promise that eventually resolved only to true or false. Even with that convenience, a returned promise will not survive an upgrade, so the wallet would need to cope with the normal upgrade-invalidated promise pattern, where it asks for a new promise in that case.

[dckc]

The misleading "Swap successfully completed." is shown in the dapp (amm.agoric.net), not the wallet.

Are you suggesting that every dApp developer should inspect the payouts rather than the offer result to get... um... the result of the offer?

Wouldn't it be much more straightforward for the AMM contract to throw in this case? I use the AMM API quite a bit, and remembering that "Swap successfully completed." actually means "swap may or may not have successfully completed; you better look more closely" is a royal pain.

At least we should change the message to "Swap order processed. Check payouts for results."

[Tartuffo]

As discussed in Eng Sync today, we need to have the correct feedback to the user, to be implemented between the UI and the AMM contract (not a fundamental change to Zoe).

[dckc]

regarding my suggestion to have the AMM contract throw, @erights pointed out a risk of relying on the contract for this info: it could lie by reporting one thing in the offer result and another in the payouts. So the client has to check the payouts, one way or another.

[dckc]

I got bit by this again, this time with the PSM, when I tried to trade 30 IST for USDC but the PSM only had 20 USDC in its piggy bank.

[dckc]

@erights points out:

UserSeat numWantsSatisfied gives a promise that settles once the seat has exited. It settles to 0 if the wants are not satisfied, and it settles to > 0 if the wants are satisfied. Currently (pre multiples) the wants are either not satisfied (0) or satisfied exactly once (1). But to be future compat better to test for 0 vs > 0.
Offer safety guarantees that if the wants are not satisfied (0) then all the escrowed assets are refunded. But note that even if the wants are satisfied, you may still receive a full refund anyway. That’s why the relevant UI question is usually whether the wants are satisfied.

[dckc]

This has been addressed for the purpose of the PSM / wallet release.