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

Refactor global sdk.Config #7448

Open
1 of 6 tasks
Tracked by #18022
aaronc opened this issue Oct 2, 2020 · 30 comments
Open
1 of 6 tasks
Tracked by #18022

Refactor global sdk.Config #7448

aaronc opened this issue Oct 2, 2020 · 30 comments
Assignees
Labels

Comments

@aaronc
Copy link
Member

aaronc commented Oct 2, 2020

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

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
@alexanderbez
Copy link
Contributor

ACK

@jazg
Copy link

jazg commented Nov 13, 2020

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

@alexanderbez
Copy link
Contributor

Not sure on timelines here, but we're welcome to reviewing PRs if you believe you have a good understanding of the proposed solution.

@clevinson
Copy link
Contributor

Blocked on #9293

@tac0turtle
Copy link
Member

is this done @JulianToledano

@JulianToledano
Copy link
Contributor

JulianToledano commented Jun 26, 2024

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.

return cacheBech32Addr(GetConfig().GetBech32AccountAddrPrefix(), aa, accAddrCache, key)

@julienrbrt
Copy link
Member

Is this completed @JulianToledano ?

@JulianToledano
Copy link
Contributor

Is this completed @JulianToledano ?

Nope, most of the calls have been removed but there're still some that may need some refactors. For example there are a lot of ValidateBasic() that relies on the String() method.

For example govs MsgExecLegacyContent

// ValidateBasic implements the sdk.Msg interface.
func (c MsgExecLegacyContent) ValidateBasic() error {
_, err := sdk.AccAddressFromBech32(c.Authority)
if err != nil {
return err
}
return nil
}

but there are more such as banks Balance and Input, evidence MsgSubmitEvidence plus some group messages.
The string method also makes it as key in some collections.

AccAddressKey collcodec.KeyCodec[AccAddress] = genericAddressKey[AccAddress]{
stringDecoder: AccAddressFromBech32,
keyType: "sdk.AccAddress",
}
// ValAddressKey follows the same semantics as AccAddressKey.
ValAddressKey collcodec.KeyCodec[ValAddress] = genericAddressKey[ValAddress]{
stringDecoder: ValAddressFromBech32,
keyType: "sdk.ValAddress",
}
// ConsAddressKey follows the same semantics as ConsAddressKey.
ConsAddressKey collcodec.KeyCodec[ConsAddress] = genericAddressKey[ConsAddress]{
stringDecoder: ConsAddressFromBech32,
keyType: "sdk.ConsAddress",
}

Furthermore all addresses unmarshall methods relies on the String method:

cosmos-sdk/types/address.go

Lines 234 to 258 in eeeb5b8

// MarshalYAML marshals to YAML using Bech32.
func (aa AccAddress) MarshalYAML() (interface{}, error) {
return aa.String(), nil
}
// UnmarshalJSON unmarshals from JSON assuming Bech32 encoding.
func (aa *AccAddress) UnmarshalJSON(data []byte) error {
var s string
err := json.Unmarshal(data, &s)
if err != nil {
return err
}
if s == "" {
*aa = AccAddress{}
return nil
}
aa2, err := AccAddressFromBech32(s)
if err != nil {
return err
}
*aa = aa2
return nil
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🤸‍♂️ In Progress
Development

No branches or pull requests