Rounding issues

[ueco-jb]

Note (Ethan): Please tackle #47 first, as mentioned in my comment

Solve them, once and for all!

Some operations currently might panic (in tests):

suite.repay(borrower, coin(1, base_asset)).unwrap();

might panic with:

panicked at 'called `Result::unwrap()` on an `Err` value: Cannot process zero tokens

or

assert_eq!(
    suite.query_ltoken_info().unwrap().total_supply,
    DisplayAmount::raw(4616u128)
);
...
suite.withdraw(lender, 4616).unwrap();
assert_eq!(suite.query_asset_balance(lender).unwrap(), 4616);
assert_eq!(
    suite.query_ltoken_info().unwrap().total_supply,
    DisplayAmount::raw(1u128)
);

One of many examples. Basically every place that contains calulations that included fractions is prone to errors. Especially observable while doing interest rate charge computations.

Ethan suggested changing MULTIPLIER 1% to 1 permille (1/1000) for better "accuracy": https://github.com/confio/lendex/blob/main/contracts/lendex-token/src/contract.rs#L53 although this is still not perfect and rounding errors still occues (rarely though).

[ethanfrey]

Let's update the interest calculation from https://github.com/confio/lendex/issues/47 before starting this issue, as that may change some of the accumulated errors.

Some concrete proposals we can try:

• [ ] Make the epoch time nicely divisible again (epoch_time usually fits neatly into 1 day - eg 5 minutes, 2 hours, etc). Let's use 365 days for the year length estimation for less rounding, rather than try to over-precise it as 365.24
• [ ] Initial MULTIPLIER to 100,000 (that is 100,000 shares to 1 token), which should provide more accuracy
• [ ] Verify panicked at 'calledResult::unwrap()on anErrvalue: Cannot process zero tokens no longer happens when trying to send 1 Token
• [ ] Write a few test cases that show the rounding errors we still have (hopefully no more than 1 token)
• [ ] Verify that accruing interest every epoch_length for 100 times will produce a total collected interest (bToken balance) >= collecting it one time after 100 epoch_length. Basically, that there is no attack vector to accrue interest often to lower your debt load.

This should "fix" the major problems associated with this issue. And those test cases showing where rounding still happens will serve as documentation if we really want to root out the last issue.

[ueco-jb]

Another idea - as likely some rounding issue will pop anyway, create a function/macro similar to assert_eq, which will include some offset to include that rounding error (currently it's 1-2 top):

// offset = 0.2%
assert_eq_with_offset!(1000, 1002, offset)

Just as a helper in tests.

[uint]

Let's do this after #109 since that PR affects the calculations.

[uint]

At this point, this is about reviewing if we still have significant rounding issues. Minor ones are expected.

[ueco-jb]

Initial MULTIPLIER to 100,000 (that is 100,000 shares to 1 token), which should provide more accuracy

[ueco-jb]

Demo with simple frontend has shown that this is still an issue. To the point, where depositing and immediate withdrawal of 10000 tokens ended up with bug, because available amount was actually 9999.

Couple loose ideas: 0) We should stick to the rule - perform as least (or as late) as possible changing denoms and corresponding amounts. 1) Review token contract implementation, so that we perform as few multiplications as possible, for example here. https://github.com/confio/isotonic/blob/main/contracts/isotonic-token/src/contract.rs#L104-L115 Maybe we could cut some stuff down, or simplify it in specific cases. 2) Expand our tests, so that for example after 100 iterations of transactions from A to B and reversed account balances are exactly the same.

[ethanfrey]

To the point, where depositing and immediate withdrawal of 10000 tokens ended up with bug, because available amount was actually 9999.

I think we will always have off by 1 rounding. The issue we had earlier is when I saw 3 or 4 units, which seemed to amplify over time.

If you do 100 trades and end up off by 100, that is more of an issue than doing 100 trades and being off by 1 or 2. We cannot reduce rounding errors below 1, but let's try to keep them from accumulating and compounding

[ueco-jb]

I understand the cause and that it will happen, but still - I'm trying to figure out how should we accomodate the fact, that user might not be able to withdraw all tokens he just deposited. It's not even about interest rates that changes the amounts.

We don't want frontend to adjust any values, right? I wonder how we could mitigate that rounding issues, so at least user is able to perform operations on actual amounts that are displayed.

[ethanfrey]

If I deposit 100 ATOM and withdraw 99.999999 ATOM, that is a bit odd, but not a major problem IMO.

If I wait a few days, it will be 100.102315 or something like that anyway.

People are used to paying a few tokens in gas every transaction, so a loss of a millionth or two is not an issue... unless that builds up to 0.1 loss after a few months (or letting the tokens sit)

[ueco-jb]

If I deposit 100 ATOM and withdraw 99.999999 ATOM, that is a bit odd

What I'm considering:

• should we force user to actually type 99.9999? (as it is now)
• should we allow withdrawing deposited 100, but actually transferring 99.9999 from contract side?
• should that be a frontend issue, they should round values and "translate" that withdraw 100 to withdraw 99.9999

because for me user's convinence should probably come first

[ethanfrey]

Ah, I see the issue.

Let's figure this out when we start integrating the frontend, and figure out where they need to do this. Maybe there is a "max" button that just prefills the largest amount (via query) and we should ensure that works.

[ueco-jb]

There's still an issue I mentioned and Ethan reflected: https://github.com/confio/isotonic/issues/40#issuecomment-1077917177 I think this still could/should be somehow tracked, because it's not yet decided whether that should be frontned or contracts

[uint]

Aaaand I've just proven there's still a problem with a deposit(x) -> withdraw(x) scenario in some cases. Reopening.

https://github.com/confio/isotonic/blob/9c7830fb38d2d985332a42caef56a71b761259e9/tests/tests/system.rs

The problem seems to be around how we calculate the transferable amount. Maybe some kind of edge case code is in order?

Suggestion: If Alice has 0 debt in TotalCreditLine, transferable_amount simply returns ∞ without doing pointless division?