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.

Sign up for GitHub

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

Jump to bottom

refactor(staking): remove account address bech32 global #15598

Merged
merged 14 commits into from
Apr 2, 2023
Merged

refactor(staking): remove account address bech32 global #15598

merged 14 commits into from
Apr 2, 2023

Conversation

tac0turtle
Copy link
Member

Description

ref #13140

remove account bech32 global from staking

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 29, 2023
View reviewed changes
x/staking/keeper/delegation.go
delegatorAddress := sdk.MustAccAddressFromBech32(red.DelegatorAddress)
delegatorAddress, err := k.authKeeper.StringToBytes(red.DelegatorAddress)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
x/staking/keeper/slash.go
delegatorAddress := sdk.MustAccAddressFromBech32(redelegation.DelegatorAddress)
delegatorAddress, err := k.authKeeper.StringToBytes(redelegation.DelegatorAddress)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
x/staking/keeper/val_state_change.go
delegatorAddress := sdk.MustAccAddressFromBech32(dvvTriplet.DelegatorAddress)
delegatorAddress, err := k.authKeeper.StringToBytes(dvvTriplet.DelegatorAddress)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
x/staking/keeper/delegation.go
delegatorAddress := sdk.MustAccAddressFromBech32(delegation.DelegatorAddress)
delegatorAddress, err := k.authKeeper.StringToBytes(delegation.DelegatorAddress)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
x/staking/keeper/delegation.go
delegatorAddress := sdk.MustAccAddressFromBech32(delegation.DelegatorAddress)
delegatorAddress, err := k.authKeeper.StringToBytes(delegation.DelegatorAddress)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
x/staking/keeper/delegation.go
delAddr := sdk.MustAccAddressFromBech32(ubd.DelegatorAddress)
delAddr, err := k.authKeeper.StringToBytes(ubd.DelegatorAddress)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
x/staking/keeper/delegation.go
delegatorAddress := sdk.MustAccAddressFromBech32(ubd.DelegatorAddress)
delegatorAddress, err := k.authKeeper.StringToBytes(ubd.DelegatorAddress)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
x/staking/keeper/delegation.go
delegatorAddress := sdk.MustAccAddressFromBech32(red.DelegatorAddress)
delegatorAddress, err := k.authKeeper.StringToBytes(red.DelegatorAddress)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
x/staking/keeper/val_state_change.go
delegatorAddress := sdk.MustAccAddressFromBech32(dvPair.DelegatorAddress)
delegatorAddress, err := k.authKeeper.StringToBytes(dvPair.DelegatorAddress)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
@tac0turtle tac0turtle marked this pull request as ready for review March 31, 2023 09:09
@tac0turtle tac0turtle requested a review from a team as a code owner March 31, 2023 09:09
julienrbrt
julienrbrt reviewed Mar 31, 2023
View reviewed changes
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it r4r? nothing to change in CLI this time?

cosmos-sdk/x/staking/client/cli/query.go

Line 246 in 368efb0

bech32PrefixAccAddr := sdk.GetConfig().GetBech32AccountAddrPrefix()

x/staking/keeper/delegation.go Show resolved Hide resolved
@julienrbrt julienrbrt changed the title chore: remove account address bech32 global from staking refactor(staking): remove account address bech32 global Mar 31, 2023
@github-actions github-actions bot added the C:CLI label Mar 31, 2023
@tac0turtle
Copy link
Member Author

is it r4r? nothing to change in CLI this time?

cosmos-sdk/x/staking/client/cli/query.go

Line 246 in 368efb0

bech32PrefixAccAddr := sdk.GetConfig().GetBech32AccountAddrPrefix()

added for cli, not sure how to grab the prefix for the example.

facundomedica
facundomedica approved these changes Mar 31, 2023
View reviewed changes
x/staking/client/cli/query.go Outdated Show resolved Hide resolved
@facundomedica
Copy link
Member

added for cli, not sure how to grab the prefix for the example.

Does it make sense to add a method that returns the prefix? Although I don't know if it's worth it just for the examples

julienrbrt
julienrbrt approved these changes Mar 31, 2023
View reviewed changes
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@julienrbrt
Copy link
Member

added for cli, not sure how to grab the prefix for the example.

Does it make sense to add a method that returns the prefix? Although I don't know if it's worth it just for the examples

They are still accessible with simd debug prefixes but maybe it does not matter in examples indeed.

@julienrbrt
Copy link
Member

Is there a plan to remove it for validators addresses as well?

@tac0turtle
Copy link
Member Author

Is there a plan to remove it for validators addresses as well?

yes want to do 1 by 1 to better track the changes

@tac0turtle tac0turtle merged commit 7f0e05e into main Apr 2, 2023
@tac0turtle tac0turtle deleted the marko/bech32_global_staking branch April 2, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facundomedica facundomedica facundomedica approved these changes

@julienrbrt julienrbrt julienrbrt approved these changes

@atheeshp atheeshp Awaiting requested review from atheeshp

Assignees
No one assigned
Labels
C:CLI C:x/staking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tac0turtle @facundomedica @julienrbrt