cosmos-sdk
cosmos-sdk copied to clipboard
cosmos
Refactor global sdk.Config
Summary
Remove global bech32 prefixes and the rest of the global config before v1.0 (see #7421).
Problem Definition
Global config variables are generally a bad design decision. And in interchain world having more flexibility around bech32 prefixes is probably desirable.
If we are targeting a v1.0 release at some point, we likely want to remove the global Config and do something better that allows us to keep the core SDK minimal, well-designed and the other parts modular.
#7242 likely brings us closer to being able to do this because address parsing is now happening inside handlers rather than at the encoding level.
Proposal
- [x] Remove bech32 https://github.com/cosmos/cosmos-sdk/issues/9690 / https://github.com/cosmos/cosmos-sdk/issues/13140
- [ ] Remove sdk.Config
See initial proposal
In #7242, addresses are now parsed manually using sdk.AccAddressFromBech32, etc. Instead we can either pass the parameters into keepers (like we do for codecs) or attach them to sdk.Context and maybe decode address using ctx.AccAddressFromBech32 in keepers and handlers.
Also we may consider removing ValAddress and ConsAddress from types/ as they are specific to x/staking.
For Admin Use
- [ ] Not duplicate issue
- [ ] Appropriate labels applied
- [ ] Appropriate contributors tagged
- [ ] Contributor assigned/self-assigned
@aaronc @alexanderbez Do we have an ETA for this? Our project currently only supports Terra so it's not a major problem (we just set the prefix for the global config on boot), but it would cause issues (e.g. concurrent map read/write panics) if we choose to support more Cosmos-compatible chains in the future.
Not sure on timelines here, but we're welcome to reviewing PRs if you believe you have a good understanding of the proposed solution.
Edit: This issue is about bech32 prefixes and sdk.Config. The 1st part, strictly about bech32 prefixes, is tracked here: https://github.com/cosmos/cosmos-sdk/issues/9690
Wanted to ping this issue and see if we have a resolution anywhere on the roadmap? This is something that is still causing pain for clients.
We have a plan for this and partially implemented it but paused because we do not want to introduce more major breaking changes into 0.45, and punted it to 0.46
As the user primarily affected by this its been hard to see this punted again and again. This effectively makes a clean relayer implementation (or any kind of concurrent cross chain workload) impossible using the go bindings. Between this issue and the upcoming breaking changes w/o a way to support additional versions (https://github.com/cosmos/cosmos-sdk/discussions/10162) building clients supporting more than one chain in the language of the sdk is becoming close to impossible w/o a massive team. Would like to not see this happen and as one of the major builders in this area I feel the need to speak up.
I've also been working on a tax reporting tool that has run into the version issue in a major way. I have 3 different versions of the same code in different repos to deal with this.
Bech32 prefixes have been an issue for a LONG time. It's a balancing act between breaking every module which depends on them in the next release and getting other high priority features out the door.
Where are you having issues talking to multiple chains with the same SDK version? Clients should be compatible other than this bech32 issue.
I'm interested to hear which "high priority features" got prioritized as the discussions on this have been held at times I've been unable to attend. The bech 32 issue has been something that was going to be worked on since this issue was opened. It has negatively affected the go client ecosystem significantly.
We are rapidly entering a world in which there are multiple chains at different versions which need to be supported by a common client. The current codebase has no way to accomodate this.
I'm interested to hear which "high priority features" got prioritized as the discussions on this have been held at times I've been unable to attend. The bech 32 issue has been something that was going to be worked on since this issue was opened. It has negatively affected the go client ecosystem significantly.
Principally #9406 lining up with Gravity Bridge was prioritized over this.
We are rapidly entering a world in which there are multiple chains at different versions which need to be supported by a common client. The current codebase has no way to accomodate this.
Can you please explain how the current codebase does not accommodate this? As far as I can tell it should and does.
Where is the discussion on gravity bridge by the way? I've been working on that project for almost a year and have 0 visibility into these discussions.
How to import different SDK versions into the same codebase? AFAIK its not possible.
Where is the discussion on gravity bridge by the way? I've been working on that project for almost a year and have 0 visibility into these discussions.
In the roadmap call we discussed lining up meta-transactions close to the gravity bridge release if possible.
How to import different SDK versions into the same codebase? AFAIK its not possible.
No it's not possible and can't be solved by the proposal #10162. We'd like it to be possible but it's just not so simple and we'll obviously try to work on it. But what should this have to do with clients? The client proto files should have been changed in backward compatibile ways so clients should be compatible. If you're having issues, again please report because the expected behavior is that newer clients (go/js/whatever) can talk to older chains.
My understanding is that if we fixed the issue with the proto generation we would be able to import github.com/cosmos/cosmso-sdk/v44 as well as github.com/cosmos/cosmos-sdk/v42. But it sounds like this work has fallen to the meta-transaction support for the unspecified gravity bridge? Again promoting speculative uses rather than existing clients with userbases. https://github.com/cosmos/cosmos-sdk/discussions/10162#discussioncomment-1369645
My understanding is that if we fixed the issue with the proto generation we would be able to import
github.com/cosmos/cosmso-sdk/v44as well asgithub.com/cosmos/cosmos-sdk/v42.
It's not an easy fix because of the SDK's dependency graph and the way proto files are registered and that scenario won't be possible without A MASSIVE amount of refactoring.
But it sounds like this work has fallen to the meta-transaction support for the unspecified gravity bridge?
It's not just for Gravity Bridge, the meta transaction work is desired by Gravity Dex and other IBC use cases. Not making a lot of major breaking API changes was a decision made for having a feature release in such a way that it didn't break a lot of code for down stream integrators such as Gravity DEX and bridge.
Yes, I've asked for help from @aaronc to ensure sure the large codebases of gravity bridge, gravity dex, and the farming and budget modules don't need to do a large refactors if/when a v0.46 is combined into the theta upgrade estimated in november. The meta-transaction was included as a priority issue based on client feedback from the emeris team to support the gravity dex.
I'm sorry that refactoring global bech32 prefixes have been a pain to client developers for such a long time. I'm not super familiar with the issue but just reading Global config variables are generally a bad design decision. And in interchain world having more flexibility around bech32 prefixes is probably desirable. sounds like a nice-to-have. It sounds like you have a different opinion about how this should be prioritized and I'd love to understand more. You're on the SDK prioritization call @jackzampolin that's at 11am EST every 4th Tuesday, early for PST but I wasn't aware that this wasn't a possible time for you. Your feedback is valued and I'd like to make sure you're able to join these calls to make sure we have the right evaluations of priorities. The next call is October 12th, please DM me if you want to request a schedule change.
I am unable to make a functioning relayer in go w/ this issue currently. About 1/3 of all relay calls on production networks fail. I've tried a number of ways around that. Also that time is still the IBC call time which has been the same issue I've had for a while.
Just following up on this issue. We've encountered into this issue as well, and implemented not-so-idea fix based on how the relayer has fixed this. Is this being worked on currently?
I've recently almost entirely scrubbed the usage of the global config away from client (https://github.com/cosmos/cosmos-sdk/pull/17503) except for ledger, that requires GetFullBIP44Path.
Where would this be defined if we remove the global config?
I've recently almost entirely scrubbed the usage of the global config away from client (#17503) except for ledger, that requires
GetFullBIP44Path. Where would this be defined if we remove the global config?
Is this still relevant ? After a quick search in the sdk repo I see that GetFullBIP44Path is only being used in tests related files. That func could be moved to /testutil without any trouble.
Is there another usage of that func that I'm not aware of ?
Is this still relevant ? After a quick search in the sdk repo I see that
GetFullBIP44Pathis only being used in tests related files. That func could be moved to/testutilwithout any trouble. Is there another usage of that func that I'm not aware of ?
Mmh, that could work indeed. There is this usage as well, https://github.com/cosmos/cosmos-sdk/blob/a556bc4a88f8c90592303502578261da72ad4709/client/keys/add.go#L82 and https://github.com/cosmos/cosmos-sdk/blob/a556bc4a88f8c90592303502578261da72ad4709/client/keys/parse.go#L83, and then it's out of client/.
Are you willing to refactor that?
Hey guys!
I think only bech32 prefixes are left to remove. I've been checking it, and it looks like Address (AccAddress, ValAddress...) types are heavily coupled to the config, as they rely on it for their String methods. Same with x/auth/config for the initialisation of the default signing options.
Not sure what the best approach is to resolve this. I guess those prefixes can be passed to the x/auth somehow, but Address types may need to be refactored. I don't believe passing an argument to a string method is a good option.
@julienrbrt @tac0turtle
Staking and auth have a module config which contains the address prefix. We already use that when using depinject.
https://github.com/cosmos/cosmos-sdk/blob/2a6640d/proto/cosmos/staking/module/v1/module.proto#L21-L25 https://github.com/cosmos/cosmos-sdk/blob/2a6640d/proto/cosmos/auth/module/v1/module.proto#L13-L14
It would be cool if those two modules when not using depinject could make use of that config. Then I believe we can fully remove the prefixes (we may still need to keep the Cons/Val/AccAdressFromBech32 and find a way to use the module config there too).
is this done @JulianToledano
nope, we need to deprecate the address string method and then remove it from the sdk. Since those make use of the global config. https://github.com/cosmos/cosmos-sdk/blob/38c1d6a5d49b86441636866f8e97de06de7f4803/types/address.go#L300