Description
Summary of Bug
Calling Take() when the basket's exponent is larger than the credit type's precision results in a failure when later trying to retire or transfer credits with max decimal place exceeded error.
Version
v3.0 & master
Steps to Reproduce
9214fda#diff-abbfa1a25e095dc4c5a102e5393dce0c76a4ffcb9f22de531b0e3925302f9316R216-R220
// take.go:216:
// Error Trace: take.go:216
// Error: Received unexpected error:
// 999999999999997.000000000100 exceeds maximum decimal places: 6
// Test: TestTakeFromBasket/TestBasketScenario
Proposed Solution
It seems like this was maybe an oversight in our design of credit basket exponent & credit type's precision?
@aaronc when we decided to allow for a custom exponent for a basket, different (but >=) the precision of a credit type, how did we imagine the handle the case of calling Take() with a smaller amount of credits than fits into precision?
It seems strange to have small decimal portions of basket tokens potentially scattered btw different users' accounts, and these all being backed by ecocredits that are essentially unredeemable unless users aggregate their micro amounts of basket tokens to equal a unit greater than or equal to the precision of the underlying ecocredit's credit type.
I see a few options:
1. Continue with existing design, but improve error handling
We could decide that this is expected behavior, and return a better error to the user like "credit type "Carbon" has max decimal places 6, and Take() was called with an amount of ecocredits with too high precision (0.000000100)"
We should ensure that we have non-failing test cases of exponent > precision, to verify that if exponent=9, precision=6, then I could still call take with 1000 basket tokens, and this would result in receiving 1 ecocredit.
Background:
Given a credit type with precision "6"
And a basket with exponent "9"
And alice has "2000" basket tokens
Scenario: alice calls take for too few ecocredits
When alice exchanges "100" basket tokens using take
Then expect the error "cannot exchange 100 basket tokens for 0.000000100; exceeds 'Carbon' credit type max precisions of 6"
Scenario: alice calls take for too precise ecocredits
When alice exchanges "1500" basket tokens using take
Then expect the error "cannot exchange 1500 basket tokens for 0.000001500; exceeds 'Carbon' credit type max precisions of 6"
Scenario: alice calls take for a valid amount of ecocredits
When alice exchanges "1000" basket tokens using take
Then expect no error
2. Succeed on Take() and only actually perform take on the number of credits rounded down to credit type precision.
Change expected behavior to something like the following:
Background:
Given a credit type with precision "6"
And a basket with exponent "9"
And alice has "2000" basket tokens
Scenario: alice calls take for too few ecocredits
When alice exchanges "100" basket tokens using "take"
Then expect no error
// or maybe we do have an error similar to when you call take with 0 basket tokens
Scenario: alice calls take for enough ecocredits
When alice exchanges "1500" basket tokens using "take"
Then alice has "1500" basket tokens
And alice has "0.000001" ecocredit3. Change design so exponent must equal precision
We could refactor baskets and require that exponent equals precision. Where this gets trick though is that our design intention is currently to have precision be upgradable, so how would we handle cases where the precision changes and how that reflects all the existing basket credits? Seems complicated...
4. Change Take() API to input number of ecocredits to take as opposed to number of basket tokens to exchange
When writing out the acceptance test above, I was reminded at how clunky and weird our language around "Take" is.
Generally it make sense to have:
- "I put 5.234 ecocredits in the basket" equate to
Put(5.234 ecocredits)
However, for for take, we have the input in the RPC call be "the asset you have", which is not ecocredits, but is instead the basket tokens.
So for Take() we have:
- "I take 5.234 ecocredits from the basket" equates to
Take(5234000 basketTokens)
But you're not "taking basket tokens", you're "taking ecocredits". If we want to have a function where the input is "basket tokens" we should probably call it Exchange() instead of Take(), as I "exchange basket tokens for ecocredits" equates better to Exchange(5234000 basketTokens)
That being said, if we want to keep the RPC call as Take() I think it would be more accurate to have the input be the amount of ecocredits that you desire to take from the basket, instead of the number of basket tokens that you are exchanging for accredits.
So if we have "I take 5.234 ecocredits from the basket" result in Take(5.234 ecocredits), then we have a cleaner version of the (1) solution described above. Where we focus on improved error handling, and accept the fact that sometimes users will have minuscule amounts of basket tokens which cannot be redeemed (in cases where basket exponent > credit type precision)
The issue with this approach is that we actually don't know which ecocredits (e.g. which specific batch) you will receive. So we would have to change the API to be something like Take(basketName, ecocreditAmount) where ecocreditAmount was a decimal that verified against the precision supported by that credit type.
Summary
My vote is for (4). The API naming around Take() is something that had been bothering me for some time, but I hadn't quite been able to put my finger on why. I think this is our best solution.
For Admin Use
- Not duplicate issue
- Appropriate labels applied
- Appropriate contributors tagged
- Contributor assigned/self-assigned
Activity