fix: ValidatorStore interface doesn't match StakingKeeper #17164

facundomedica · 2023-07-27T14:34:33Z

Description

We had to add the necessary functions directly to the Staking Keeper given that when you have an interface with a function that returns another interface, you can't make another interface comply because the returning interface won't be accepted (example: https://go.dev/play/p/tz50jVl0NBw).

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)

julienrbrt · 2023-07-27T14:51:47Z

I don't know how we can make sure these interfaces match without creating new dependency, but it would be good to do it somehow (maybe implementing it on Simapp).

We could just have a function call in /integration/ to, it does not pollute simapp.

julienrbrt

We should update the ADR as well

cosmos-sdk/docs/architecture/adr-064-abci-2.0.md

Line 220 in bd61e84

    
           GetValidatorByConsAddr(sdk.Context, cryptotypes.Address) (cryptotypes.PubKey, error)

baseapp/abci_utils.go

facundomedica · 2023-07-27T15:28:40Z

Moving this to draft as I'm writing an integration test

tac0turtle · 2023-07-27T17:22:45Z

I don't know how we can make sure these interfaces match without creating new dependency, but it would be good to do it somehow (maybe implementing it on Simapp).

can we import baseapp for now into staking to make sure it matches, later on we can fix the dependency but for now i think its fine

alexanderbez · 2023-07-27T19:04:08Z

I don't know how we can make sure these interfaces match without creating new dependency, but it would be good to do it somehow (maybe implementing it on Simapp).

can we import baseapp for now into staking to make sure it matches, later on we can fix the dependency but for now i think its fine

I wanted to avoid this if possible honestly. I think we tweak the API(s) to avoid this but if we cannot, then yes, this is a fallback

facundomedica · 2023-07-27T19:28:45Z

@tac0turtle @alexanderbez I just saw your comments, in my last commit I've added the necessary APIs to the staking keeper + an integration test. lmk what you think

julienrbrt

lgtm!

julienrbrt · 2023-07-28T13:50:10Z

baseapp/abci_utils.go

@@ -125,12 +107,20 @@ func ValidateVoteExtensions(
 			return fmt.Errorf("failed to verify validator %X vote extension signature", valConsAddr)
 		}

-		sumVP = sumVP.Add(validator.BondedTokens())
+		bondedTokens, err := valStore.BondedTokensByConsAddr(ctx, valConsAddr)


Curious, isn't it slower now because we get the validator twice? While before only once.
It is probably negligible but just curious. :D

Yes, it will be slower sadly. We could have a specific API that returns both bonded tokens and public key, wdyt?

No objection to that personally.

@facundomedica yes let's do that. This will be used in the VE hotpath so we don't want obvious performance slowdowns

Added BondedTokensAndPubKeyByConsAddr

…/fix-valvoteext

x/staking/keeper/validator.go

docs/architecture/adr-064-abci-2.0.md

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit c3ae0b0) # Conflicts: # CHANGELOG.md

…17164) (#17283) Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

fix: ValidatorStore interface didn't match StakingKeeper

4270b69

facundomedica requested a review from a team as a code owner July 27, 2023 14:34

This comment has been minimized.

Sign in to view

julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Jul 27, 2023

julienrbrt reviewed Jul 27, 2023

View reviewed changes

baseapp/abci_utils.go Outdated Show resolved Hide resolved

facundomedica marked this pull request as draft July 27, 2023 15:28

add necessary functions to staking keeper + integration test

e0ef985

github-actions bot added the C:x/staking label Jul 27, 2023

facundomedica added 2 commits July 28, 2023 11:00

lint and cleanup

8ea7b79

update adr

c08742f

github-actions bot added the Type: ADR label Jul 28, 2023

facundomedica marked this pull request as ready for review July 28, 2023 12:04

cl++

9ba8443

julienrbrt approved these changes Jul 28, 2023

View reviewed changes

facundomedica added 2 commits July 29, 2023 19:28

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu…

ccda6cf

…/fix-valvoteext

use BondedTokensAndPubKeyByConsAddr

1fb12d7

github-actions bot added the C:x/gov label Jul 29, 2023

fix test

f5ff374

julienrbrt approved these changes Jul 29, 2023

View reviewed changes

x/staking/keeper/validator.go Outdated Show resolved Hide resolved

docs/architecture/adr-064-abci-2.0.md Show resolved Hide resolved

fixes

72bbdfd

julienrbrt mentioned this pull request Aug 3, 2023

Cut v0.50.0-rc.0 #17282

Closed

3 tasks

alexanderbez approved these changes Aug 3, 2023

View reviewed changes

Merge branch 'main' into facu/fix-valvoteext

e38647d

alexanderbez enabled auto-merge August 3, 2023 22:41

github-actions bot removed the C:x/gov label Aug 3, 2023

alexanderbez added this pull request to the merge queue Aug 3, 2023

Merged via the queue into main with commit c3ae0b0 Aug 3, 2023
48 checks passed

alexanderbez deleted the facu/fix-valvoteext branch August 3, 2023 22:55

mergify bot pushed a commit that referenced this pull request Aug 3, 2023

fix: ValidatorStore interface doesn't match StakingKeeper (#17164)

0261375

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit c3ae0b0) # Conflicts: # CHANGELOG.md

mergify bot mentioned this pull request Aug 3, 2023

fix: ValidatorStore interface doesn't match StakingKeeper (backport #17164) #17283

Merged

julienrbrt mentioned this pull request Aug 24, 2023

fix(baseapp): Utilizing voting power from VEs in ValidateVoteExtensions #17518

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ValidatorStore interface doesn't match StakingKeeper #17164

fix: ValidatorStore interface doesn't match StakingKeeper #17164

facundomedica commented Jul 27, 2023 •

edited

Loading

This comment has been minimized.

julienrbrt commented Jul 27, 2023

julienrbrt left a comment

facundomedica commented Jul 27, 2023

tac0turtle commented Jul 27, 2023

alexanderbez commented Jul 27, 2023

facundomedica commented Jul 27, 2023

julienrbrt left a comment

julienrbrt Jul 28, 2023

facundomedica Jul 28, 2023

julienrbrt Jul 28, 2023

alexanderbez Jul 28, 2023

facundomedica Jul 29, 2023

fix: ValidatorStore interface doesn't match StakingKeeper #17164

fix: ValidatorStore interface doesn't match StakingKeeper #17164

Conversation

facundomedica commented Jul 27, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

This comment has been minimized.

julienrbrt commented Jul 27, 2023

julienrbrt left a comment

Choose a reason for hiding this comment

facundomedica commented Jul 27, 2023

tac0turtle commented Jul 27, 2023

alexanderbez commented Jul 27, 2023

facundomedica commented Jul 27, 2023

julienrbrt left a comment

Choose a reason for hiding this comment

julienrbrt Jul 28, 2023

Choose a reason for hiding this comment

facundomedica Jul 28, 2023

Choose a reason for hiding this comment

julienrbrt Jul 28, 2023

Choose a reason for hiding this comment

alexanderbez Jul 28, 2023

Choose a reason for hiding this comment

facundomedica Jul 29, 2023

Choose a reason for hiding this comment

facundomedica commented Jul 27, 2023 •

edited

Loading