Concordium / concordium-reference-wallet-android

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

Disposable amount is calculated incorrectly and can become negative #150

Closed Bargsteen closed 2 years ago

Bargsteen commented 2 years ago

Bug Description The disposable amount is calculated incorrectly, and it can become negative. I assume this is because the calculation used is disposable = total - scheduled - delegated, but the scheduled amount can be delegated and then it is subtracted twice.

Steps to Reproduce

  1. Use account A with e.g. 1000 CCD
  2. Send 600 CCD with a schedule to A
  3. Delegate 1500 CCD before the scheduled CCD are released (thereby delegating some of the scheduled CCD)
  4. Look at the disposable amount

Expected Result The disposable amount should be ~100 CCD.

Actual Result The disposable amount is negative: - ~500.

Versions

rimbi commented 2 years ago

The test could not be verified with the Android MW version 3.0.11 (95).

Expected Result The disposable amount should be ~100 CCD.

Actual Result The disposable amount is negative: 0 CCD.

XOOPsoft commented 2 years ago

@jens-concordium @rimbi I have talked to Jens Ager about this. If you have a public balance of 1000 CCD and then receive 600 in a released scheduled. Then you have 1600 in public balance, but there is still only 1000 at disposal for fees and for sending CCD to someone. So if you stake 1500, then you have staked the 1000 from your at disposal and 500 from your release schedule. It means that there is 0 CCD left in your at disposal. Is is possible for you to attach a screenshot? @jens-concordium - can you confirm if it's correct or not?

rimbi commented 2 years ago

That's not what I get from my conversations with the team. What do you think @mh-concordium and @td202 ? The result also contradicts with the expectation stated in the issue description.

jens-concordium commented 2 years ago

@rimbi @XOOPsoft It stakes the locked amounts first. So in this example it will stake the 600 in the release schedule, and then 900 from the unlocked public balance, leaving 100 at disposal.

XOOPsoft commented 2 years ago

@rimbi @jens-concordium I have talked to Jens Ager again. I will correct this issue. Thanks!

jens-concordium commented 2 years ago

Yep, this was my mistake! I had given @XOOPsoft incorrect information in the first place.

mh-concordium commented 2 years ago

Two cases to test:

  1. If you have staked/delegated your 500 CCDs that were at disposal, you will end up with 0 at disposal and 500 staked. Then, afterwards you receive 500 CCDs via scheduled transfer, you should have 500 at disposal because those which are scheduled should be staked/delegated first.
  2. If you have 500 CCDs at disposal and 500 CCDs scheduled, if you delegate 900, you should have 100 at disposal (- tx fee).
mh-concordium commented 2 years ago

@Niljak could you please implement the same for iOS?

Bargsteen commented 2 years ago

Two cases to test:

  1. If you have staked/delegated your 500 CCDs that were at disposal, you will end up with 0 at disposal and 500 staked. Then, afterwards you receive 500 CCDs via scheduled transfer, you should have 500 at disposal because those which are scheduled should be staked/delegated first.
  2. If you have 500 CCDs at disposal and 500 CCDs scheduled, if you delegate 900, you should have 100 at disposal (- tx fee).

I am not sure test case 1 aligns with how it works on the chain. Have you tested the same thing with concordium client @mh-concordium? Likely @td202 can shed some light on this ;)

UPDATE: The test cases actually came directly from a statement by @td202. So no cause for alarm. Sorry about that ;)

XOOPsoft commented 2 years ago

For the mentioned test cases. What should be displayed at the top of the "Sends funds" page for total and at disposal? And which amount should be filled in, when using the "Send all" button?

mh-concordium commented 2 years ago

@XOOPsoft, this page seems to be working fine. Why do you ask?

Total = staked/delegated + scheduled + at disposal At disposal = only what is available right now (At disposal = Total - staked/delegated - scheduled) Send all button should fill in At disposal value - fee

XOOPsoft commented 2 years ago

So there is no difference to "at disposal" if a scheduled amount is created before or after setting up delegation?

mh-concordium commented 2 years ago

Good question, it actually matters. As written in test case 1: If you have staked/delegated your 500 CCDs that were at disposal, you will end up with 0 at disposal and 500 staked. Then, afterwards you receive 500 CCDs via scheduled transfer, you should have 500 at disposal because those which are scheduled should be staked/delegated first.

I have just confirmed that behavior with Concordium Client.

XOOPsoft commented 2 years ago

Fixed in version 3.0.0 (97) (notice the version-number, it has been decided that only build-number will be increased for the release, not the 3.0.0)

rimbi commented 2 years ago

There is something wrong with the balance calculation:

Photo on 22 06 2022 at 14 52

Either Balance total should be ~1500 or total currently at disposal be ~100

verified with version 3.0.0 (97)

XOOPsoft commented 2 years ago

Fixed in release 3.0.0 (98)

rimbi commented 2 years ago

verified with release 3.0.0 (98)