refactor: remove global valaddress bech32 codec calls (1/2) by tac0turtle · Pull Request #17098 · cosmos/cosmos-sdk

tac0turtle · 2023-07-23T22:00:36Z

Description

remove calls to valAddressCodec global

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

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)

Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: Marko <marko@baricevic.me> Co-authored-by: Julien Robert <julien@rbrt.fr>

tac0turtle · 2023-07-24T15:30:51Z

while there are not many lines changed i think its worth opening this for review and ill do another pr to follow up further

julienrbrt

lgtm! Some nits.
It should still be updated here too:

cosmos-sdk/x/staking/keeper/query_utils.go

Line 134 in 158ce7c

valSrcAddr, err := sdk.ValAddressFromBech32(redelegation.ValidatorSrcAddress)

and at a few other places. However, I noticed the 1/2, so I guess it comes later :)

 		// Log the slash attempt for future reference (maybe we should tag it too)
+		conStr, err := k.consensusAddressCodec.BytesToString(consAddr)
+		if err != nil {
+			panic(err)



+	valAddr, err := k.validatorAddressCodec.BytesToString(validator.GetOperator())
+	if err != nil {
+		panic(err)


itsdevbear · 2023-07-24T19:10:56Z

 	for i := range validators {
 		validators[i], err = types.NewValidator(
-			sdk.ValAddress(addrs[i]),
+			sdk.ValAddress(addrs[i]).String(),


although this is fine, feels dirty / bad practice

oh this is 100% ugly, mainly didnt want to rewrite all these tests to use the codec as we will be rewriting this module in the near future

itsdevbear · 2023-07-24T19:11:09Z


 	// setup a new validator with bonded status
-	validator, err := types.NewValidator(valAddr, PKs[0], types.NewDescription("Validator", "", "", "", ""))
+	validator, err := types.NewValidator(valAddr.String(), PKs[0], types.NewDescription("Validator", "", "", "", ""))


same here, is just showing bad practice.

itsdevbear · 2023-07-24T19:11:18Z

 	// Produce the create validator message.
 	msg, err := stakingtypes.NewMsgCreateValidator(
-		privVal.PubKey().Address().Bytes(),
+		sdk.ValAddress(privVal.PubKey().Address()).String(),


itsdevbear

overall utACK

but one thing is that a lot of the tests still use .string() which is fine, but overall is an antipattern that is being promoted in the tests.

kocubinski · 2023-07-25T11:43:38Z


 // NewTxCmd returns a root CLI command handler for all x/staking transaction commands.
-func NewTxCmd() *cobra.Command {
+func NewTxCmd(valAddrCodec, ac address.Codec) *cobra.Command {


did you explore any other ways to provide these codecs as dependencies beside constructor injection? another approach which comes to mind is packing them into the command context.

@tac0turtle this actually brings a good point…

well the reason i went this approach is they will be removed in the near future in favour of autocli. I can look at adding it into the context but then they could be removed later on, so it would be two breaking changes instead of one then removal

Co-authored-by: Devon Bear <itsdevbear@berachain.com> Co-authored-by: Julien Robert <julien@rbrt.fr> (cherry picked from commit 58855c6) # Conflicts: # CHANGELOG.md # tests/integration/staking/keeper/validator_test.go # testutil/testnet/genesis.go # x/distribution/keeper/genesis.go # x/gov/keeper/tally_test.go # x/staking/simulation/decoder_test.go # x/staking/testutil/validator.go # x/staking/types/validator_test.go

…17098) (#17149) Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

valAddress

e341928

github-actions Bot added the C:x/staking label Jul 23, 2023

refactor(codec): Modularize Address Codec (#17099)

1a89049

Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: Marko <marko@baricevic.me> Co-authored-by: Julien Robert <julien@rbrt.fr>

github-actions Bot added C:CLI C:x/genutil genutil module issues labels Jul 24, 2023

fix tests

9f054f8

github-actions Bot added the C:Simulations label Jul 24, 2023

tac0turtle added 4 commits July 24, 2023 15:11

lint

de06bd7

fix tests

28380cc

fix e2e tests

e3c1797

abstract final call to valAddress.String in staking

23bbc21

github-actions Bot added C:x/gov C:x/slashing labels Jul 24, 2023

tac0turtle changed the title ~~refactor: remove global bech32 codec calls~~ refactor: remove global valaddress bech32 codec calls Jul 24, 2023

tac0turtle added 4 commits July 24, 2023 16:33

fix tests

b7a5dd9

remove more global calls

5f00f66

fix genutil tests

e801e48

remove more global calls

f7af2dd

github-actions Bot added the C:x/distribution distribution module related label Jul 24, 2023

remove more global calls

5d42a5e

tac0turtle added 3 commits July 24, 2023 17:32

fix some linting

ac8f2e2

fix error case in test

13cfb72

fix error case in test

d289df0

tac0turtle marked this pull request as ready for review July 24, 2023 16:01

tac0turtle requested review from a team, itsdevbear and julienrbrt July 24, 2023 16:01

ghost self-requested a review July 24, 2023 16:01

tac0turtle changed the title ~~refactor: remove global valaddress bech32 codec calls~~ refactor: remove global valaddress bech32 codec calls (1/2) Jul 24, 2023

quick add of gov

158ce7c

julienrbrt approved these changes Jul 24, 2023

View reviewed changes

julienrbrt added the backport/v0.50.x label Jul 24, 2023

tac0turtle added 2 commits July 24, 2023 18:29

changelog entry

e7eb397

change cmd line examples

425e2db

github-advanced-security AI found potential problems Jul 24, 2023

View reviewed changes

itsdevbear reviewed Jul 24, 2023

View reviewed changes

itsdevbear approved these changes Jul 24, 2023

View reviewed changes

kocubinski reviewed Jul 25, 2023

View reviewed changes

tac0turtle added 3 commits July 25, 2023 22:50

change cmd line examples

a0ba91e

query utils cleanup

8c4290f

fix gov

d31de8a

tac0turtle enabled auto-merge July 26, 2023 13:07

tac0turtle added 3 commits July 26, 2023 19:52

fix dist

a467b7a

Merge branch 'main' into marko/valAddress

7119063

fix merge conflict test failures

b6aaa22

tac0turtle added this pull request to the merge queue Jul 26, 2023

Merged via the queue into main with commit 58855c6 Jul 26, 2023

tac0turtle deleted the marko/valAddress branch July 26, 2023 19:03

mergify Bot mentioned this pull request Jul 26, 2023

refactor: remove global valaddress bech32 codec calls (1/2) (backport #17098) #17149

Merged

julienrbrt added a commit that referenced this pull request Jul 26, 2023

refactor: remove global valaddress bech32 codec calls (1/2) (backport #…

46be66e

…17098) (#17149) Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

vietanh2k mentioned this pull request Oct 22, 2024

Bump sdk v50 decentrio/multi-staking#5

Open

faddat mentioned this pull request Nov 8, 2024

faddat/many times #22476

Closed

12 tasks

Conversation

tac0turtle commented Jul 23, 2023

Description

remove calls to valAddressCodec global

Author Checklist

Reviewers Checklist

Uh oh!

tac0turtle commented Jul 24, 2023

Uh oh!

julienrbrt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Check warning

itsdevbear Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

tac0turtle Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

itsdevbear Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

itsdevbear Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

itsdevbear left a comment

Choose a reason for hiding this comment

Uh oh!

kocubinski Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsdevbear Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

tac0turtle Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

itsdevbear Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

julienrbrt left a comment •

edited

Loading

kocubinski Jul 25, 2023 •

edited

Loading