Concordium / concordium-reference-wallet-android

Reference Android wallet for the Concordium blockchain
Apache License 2.0
12 stars 2 forks source link

Handle too large delegation amounts when changing target pools #141

Closed jens-concordium closed 2 years ago

jens-concordium commented 2 years ago

Task description

If a delegator has a current delegation amount that is too large for a new target pool, the MW should handle the situation in a way that is understandable for the user. There are two scenarios where a current delegation amount would breach a new target pools limit:

  1. A delegators amount is currently in lockdown, and the delegator has chosen a new target pool ID with a limit that would be breach with the amount currently in lockdown. In this case the error message should be slightly different than it usually is. Pool limit will be breached --> Your delegation amount is too large for this pool. If possible it would also be nice to highlight the currently delegated amount at the top of the screen.

  2. A delegator has chosen a new target pool and the currently delegated amount (no cooldown) would breach the new pools limit. As the node would just reject a transaction where the current delegation amount would breach a new target pools limit (even though the user also lowered the amount in the same transaction), the user should be met with a notification explaining the situation. The notification should either let the user lower the delegation amount, stop the delegation or just cancel the flow.

Both scenarios can be see in Flow7: Update delegation transaction, Scenario 6: Current delegation amount will breach new targets limits

rimbi commented 2 years ago

The change Pool limit will be breached --> Your delegation amount is too large for this pool. is missing.

Verified with the Android MW version 3.0.11 (95)

XOOPsoft commented 2 years ago

Fixed in version 3.0.12 (96)

rimbi commented 2 years ago

Old text is still there: WhatsApp Image 2022-06-22 at 9 25 22 AM

Verified with version 3.0.12 (96)

XOOPsoft commented 2 years ago

On the screenshot you are not in cooldown (amount is not locked) When not in cooldown the text should be "Pool limit will be breached" If you are in cooldown (locked amount), then the text should be "Your delegation amount is too large for this pool" So the app is correct in the screenshot.

Please see Figma here: https://www.figma.com/file/pUlYPsHRU0Hvm0NoGIvxrv/Mobile-Wallet?node-id=3495%3A17217 and here: https://www.figma.com/file/pUlYPsHRU0Hvm0NoGIvxrv/Mobile-Wallet?node-id=4762%3A20593

rimbi commented 2 years ago

@jens-concordium can you verify @XOOPsoft 's expectation above, please?

jens-concordium commented 2 years ago

@rimbi @XOOPsoft

This depends on the scenario. I hope my explanation makes sense. If not, then we can have a call, and then I can try to explain it there.

Scenario 1: The Pool limit will be breached is still meant to be there, in the scenario where someone is trying to set up a new delegation with an amount that will breach the limit. It can also happen if a user is trying to increase their amount in the same pool.

Scenario 2: The user has an existing delegation running with no current cooldown, but wants to change target pools. The current amount fits with the new pool, but while changing target the user also wants to increase their delegation amount. In case the newly input delegation amount exceeds the new pool limit, the Pool limit will be breached text will also be shown.

Scenario 3: In case the user has an existing delegation running with no current cooldown, but tries to change target pools with their current delegation amount being too large for the new pool, the notification in the screenshot should be shown. If the user then decides to press Lower delegation amount and stay with current pool, the text should be like in Scenario 1, because the user is staying with their current pool.

Scenario 4: The user is trying to change target pools, while their current delegation amount is in cooldown. The current delegation amount is also larger than what can fit in the new pool. In this scenario, the input field should say Amount locked and the warning below should say Your delegation amount is too large for this pool.

rimbi commented 2 years ago

I see, thanks.

In the 4th scenario, I see the message Your delegation amount is too large for this pool message not on the page where the amount is entered but the previous page where new baker id is entered. Is this acceptable?

jens-concordium commented 2 years ago

I think so. Maybe I even agreed with @XOOPsoft and @Niljak that it should be like that. Was that how we ended up doing it, @XOOPsoft and @Niljak?

XOOPsoft commented 2 years ago

No, I don't recall that we talked about showing the message on the previous page. I have just created a new release 3.0.0 (97) All scenarios should be in place now.

rimbi commented 2 years ago

This fix seems to have caused another bug.

I have a stake as shown below

Photo on 22 06 2022 at 16 58

I am trying to update current delegation by leaving the baker id field empty as shown below

Photo on 22 06 2022 at 16 59

Clicking Continue results in the popup menu as follows:

Photo on 22 06 2022 at 17 01

verified with 3.0.0 (97)

XOOPsoft commented 2 years ago

Thanks for the screenshots, I will take a look.

XOOPsoft commented 2 years ago

Fixed in release 3.0.0 (98)

rimbi commented 2 years ago

Verified with release 3.0.0 (98)