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

Put sdk.Coins functions into core #13155

Open
1 task done
amaury1093 opened this issue Sep 5, 2022 · 3 comments
Open
1 task done

Put sdk.Coins functions into core #13155

amaury1093 opened this issue Sep 5, 2022 · 3 comments
Assignees

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Sep 5, 2022

Latest update


Original issue below

Summary

Carve sdk.Coin and sdk.Coins out of cosmos/cosmos-sdk, put them in core/coins.

Problem Definition

With ADR-053, we are removing core SDK components out of the root cosmos/cosmos-sdk go module, and putting them into standalone go modules: error, math, store, core etc.

Textual would like to use sdk.Coins in #12708, but because the tx module is standalone, it cannot import sdk.Coins.

Proposal

Carve sdk.Coin and sdk.Coins out of cosmos/cosmos-sdk, put them in core/coins. Add an alias in types/coins.go to point to the carved out coins.

Question: should we add Legacy prefix (similar to #12634)? as they depend on gogoproto

Further Improvements

Maybe in the future, we would migrate to pulsar coins, and use functions instead of methods:

package cossmossdk.io/core/coins

- func (coins Coins) Find(denom string) (bool, Coin) { // gogoproto coins
+ func Find(coins Coins, denom string) (bool, Coin) { // pulsar coins

but let's create another issue for that.

@aaronc
Copy link
Member

aaronc commented Sep 6, 2022

My preference would be to just add the google.golang.org/protobuf Coin functionality into core when we are ready to do that.

I see the need for some coin methods for SIGN_MODE_TEXTUAL, but just using the existing functionality in types I think is actually not correct for textual. In particular, ParseDecCoin depends on the global denom regex which I think is not right for SIGN_MODE_TEXTUAL. This denom regex is a mutable global which to begin with we should get rid of in the same way that we should get rid of the global bech32 config. Secondly, the default denom regex is actually way too restrictive for generic zero-configuration coin parsing. Also, we'll need to allocate sdk.Coins in addition to the proto v2 coins which is just unnecessary overhead for this functionality.

So I actually think we will get a more correct implementation if we just have something custom for SIGN_MODE_TEXTUAL and potentially start using that as the basis for the google.golang.org/protobuf Coin support which we'll need anyway.

@amaury1093 amaury1093 changed the title Put sdk.Coins into core Put sdk.Coins format/parse functions into core Sep 12, 2022
@amaury1093 amaury1093 self-assigned this Sep 12, 2022
@JimLarson
Copy link
Contributor

This might be a good time to incorporate the fixes contemplated in #11223.

@amaury1093 amaury1093 changed the title Put sdk.Coins format/parse functions into core Put sdk.Coins functions into core Sep 15, 2022
@joe-bowman
Copy link
Contributor

This might be a good time to incorporate the fixes contemplated in #11223.

Yes, please let's get rid of panicking when checking if two coins of different denoms are equal!

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

4 participants