Write ADR for Coins

[fedekunze]

Summary

Write ADR with design desitions for the existing Coin(s) and DecCoin(s) implementation

Problem Definition

During #7027, multiple questions where rised on the decisions behind these types:

• Differences between Coin and Coins

can we document the rational to why a single coin of ZeroInt is valid but a set of coins with a coin of ZeroInt is invalid? Seems counter-intuitive to me and if we support this it, the reason should be well documented. I'd be in favor of only considering positive amount Coins valid
@colin-axner

• Denom regex choices

Regarding unicode characters, I think we need to be a little careful - are there any two characters which look very similar / identical but are actually represented differently (i.e., are unequal)? This can be an attack vector (where some malicious agent purposefully creates a denom that looks almost exactly like a real one - e.g. atom - but is actually unequal).
@cwgoes

• Custom Denominations

#6755 and #6744 by @okwme

Proposal

Write ADR that summarizes all these choices and decisions

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[colin-axner]

I recently came across a funky bug living in x/bank, luckily the bug is very particular and is unlikely to affect implementations. In short, empty coin sets are considered valid, which x/bank did not take into account. Things like minting, sending and delegating empty coin sets do not return errors but basically do a no op. The issue lies in AddCoins and SubtractCoins. Adding or subtracting an empty coin set returns a resulting value of an empty coin set. This is counter intuitive since developers may assume the returned resulting value, is the balance of the address being added or subtracted to. I have posted a partial fix in #7327 to address immediate concerns.

Some points of discussion I think would be worthwhile to come to clear consensus on and follow up with well documented reasoning.

• should coins.IsValid() return true for empty coin sets?
• should our bank functions allow things like minting empty coin sets (we could add a check if coins.IsEmpty to prevent this)
• should we change a zero coin to return false on invalid
• should we change coins to use uints (there exist TODOs in coin.go for this but I cannot find an open issue)
• we should document reasons for the current naming restrictions (for example the character max is set to allow hashes but otherwise arbitrary)

I have come across a lot of previous discussion making excellent points for deciding one way or another. Below is just my own short reflection on these points.

IsValid on empty sets

I think it is counter intuitive to return true for this function. I think this point is demonstrated by the fact that our own implementation of bank misinterprets the semantics here. @cwgoes pointed out to me that IsValid likely returns true on empty sets since an account balance may eventually be an empty set. Perhaps this sort of context justifies this semantic reasoning? I think it'd be feasible to add a if !coins.IsEmpty() check when determining if an account balance is valid, if necessary. Regardless, this decision should be well documented in an ADR and everywhere in godoc to prevent subtle bugs.

Bank allowing empty coin set usage?

I think it could make sense to just add checks to return errors on empty coin sets in the short term.

Zero coin to return false on IsValid?

Currently a single Coin will return true on IsValid. A set of coins containing a single zero coin will return false on IsValid.

One of my first prs on the SDK #4558 asked this question as well. There was partial agreement that this should be changed, but ultimately wasn't because the change was non-trivial and at the time I did not have enough understanding of the SDK. I think this is something worth changing before 1.0

Uints for coin amounts

I think this issue requires more discussion. I found this comment, but from what I can tell, this is unimplemented? Do we have "regular coins"?

---

There appear to be a host of coin related issues still alive #2443, #2627, #3012, #3866, we weren't validating denom for multiple coins until recently, #6727, #7326.

I'd think it be spectacular if coins could get some much needed love and a face lift to ensure that one of the most security critical parts of the SDK is by far the most transparent in implementation and architecture decisions!

[alexanderbez]

Agree with most of the points there @colin-axner and thank you for writing this up.

Just to summarize my position:

should our bank functions allow things like minting empty coin sets

Absolutely not. Minting anything that's empty should not be allowed and just be ignored by an IsEmpty call.

should we change a zero coin to return false on invalid

Do you mean empty? A zero-sum coin should not be allowed anywhere in the SDK and should be considered invalid in every single case. We don't allow a zero-sum coin anywhere (i.e. it should be removed from a coin set). Is this no longer the case?

should we change coins to use uints

Maybe? I started work on this many moons ago and it's not trivial, hence the addition of safe* private methods. However, it can be done via an internal wrapper type, i.e. the public type is uint, but the internal type is int which is where all the math happens. I'm not sure if this abstraction and extra code is worth it?

[colin-axner]

Do you mean empty? A zero-sum coin should not be allowed anywhere in the SDK and should be considered invalid in every single case. We don't allow a zero-sum coin anywhere (i.e. it should be removed from a coin set). Is this no longer the case?

I'm specifically referencing the Coin type which just checks if the amount is negative. I think the SDK uses Coins everywhere so it would only be a problem for external modules using Coin instead of Coins

[alexanderbez]

Ahh right, but you should never be working with a zero-sum Coin type.

[tac0turtle]

closing as coin is already in prod, if we have another design in mind we can write a ADR