basket Take() results in eventual max precision exceeded error if basket exponent > credit precision

[clevinson]

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" ecocredit

3. 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

[ryanchristo]

I'm leaning towards (3) at the moment. We would mark exponent as deprecated in MsgCreate and then automatically set the exponent to the credit type precision. This would be the least amount of effort with no breaking changes. This would assume that credit type precision and basket exponent are not updateable in the future.

[aaronc]

Yeah I think I agree with Ryan. Exponent and precision should always be equal and not updateable. I don't see a scenario where they'd need to be updateable for our use cases. This was more of an experiment of a design for bank that handles both decimals and integers gracefully. Not really worth the overhead for ecocredits