Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch coins.Amount to uint, consider using a deterministic map instead of a list #1273

Closed
cwgoes opened this issue Jun 15, 2018 · 9 comments

Comments

@cwgoes
Copy link
Contributor

cwgoes commented Jun 15, 2018

Ref https://github.com/cosmos/cosmos-sdk/pull/1218/files#r195270328

@cwgoes cwgoes mentioned this issue Jun 15, 2018
6 tasks
@cwgoes
Copy link
Contributor Author

cwgoes commented Jun 15, 2018

Depends on #553.

@ebuchman
Copy link
Member

Unclear how we want to move forward with this.

My impressions are:

  • we need to do something about it
  • so long as we have a small number of coins, what we have is fine
  • we don't even have a way to add new coins yet, and we're launching with just one, so we're fine for a bit
  • if governance rolls out a new coin via hard-fork upgrade, this is still fine, until we have a lot of them
  • by then we're hard-forking around anyways

In conclusion, I think we can punt on actually dealing with this for now.

Anyone disagree ?

@ValarDragon
Copy link
Contributor

ValarDragon commented Jun 28, 2018

I agree that we can move deterministic map to post launch (especially since that will involve us creating our own datastructure), but I think coins.Amount should be made into a Uint prelaunch.

@jackzampolin
Copy link
Member

Is this still an issue? I'm guessing yes.

@alexanderbez
Copy link
Contributor

alexanderbez commented Nov 16, 2018

Definitely think this should be tackled pre-launch. Any opposition to this?

I like @ValarDragon's idea of using arithmetic coins (signed ints which users never really interact with) and "safe" or "regular" coins (unsigned ints that users do interact with and that is kept in state).

/cc @ebuchman @cwgoes

@ValarDragon
Copy link
Contributor

Oh sorry I forgot to write up the various things into this issue! To recap the arithmetic coins idea,

I think the result of any arithmetic should be of type "arithmetic coins". Arithmetic coins can have negative denoms, so you can easily have things like A - B + C, where its negative after (A - B) but positive at the end. You then have to cast arithmetic coins into a normal coins, i.e. arithmetic_coins.Safe_Coins (better name pending)

Whenever you parse a coin value, or write a coin value, it would be the regular coins. So state would only have these regular coins, and fees would also only be read as regular coins.

I like this because it enables fast addition laws, while also giving us the safety we'd want.

I also think the coin denominations should be sorted, not a deterministic map. The reason I am in favor of this is because it makes the addition law much quicker.

@cwgoes
Copy link
Contributor Author

cwgoes commented Nov 19, 2018

I think the result of any arithmetic should be of type "arithmetic coins".

I agree in general but I'm not sure this is worth the complexity right now, given that we don't actually perform A - B + C anywhere in the codebase (that I can find).

I also think the coin denominations should be sorted, not a deterministic map.

This is now true. Maybe this is good enough, although I still don't like that we're storing all the denomination strings repeatedly in state.

@ValarDragon
Copy link
Contributor

This is now true. Maybe this is good enough, although I still don't like that we're storing all the denomination strings repeatedly in state.

Agreed, I was actually writing another issue for exactly that last night, and just didn't get a chance to finish it then lol.

@cwgoes
Copy link
Contributor Author

cwgoes commented Jul 4, 2019

This has been done.

@cwgoes cwgoes closed this as completed Jul 4, 2019
@colin-axner colin-axner mentioned this issue Sep 16, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants