feat!: IS rebase by tac0turtle · Pull Request #13122 · cosmos/cosmos-sdk

tac0turtle · 2022-09-01T12:55:33Z

Description

Closes: #XXXX

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)

alexanderbez

Did a first pass through. I'm gonna need more time to look at the staking changes. The index key stuff I find to be pretty confusing atm.

alexanderbez · 2022-09-06T15:15:28Z

+) (val types.Validator, found bool) {
+	store := ctx.KVStore(k.storeKey)
+
+	valKey := store.Get(types.GetUnbondingIndexKey(id))


So am I correct in reading that the GetUnbondingIndexKey key can store numerous different things? I.e. validator, redelegation keys, and undelegation keys?

Exactly. The UnbondingIndexKey is a mapping from UnbondingIDs to strings, where each string is the key of an unbonding operation, i.e., validator (that is unbonding), undelegation entry, redelegation entry.

Interesting. This initially seems like a codesmell/poor design choice to me. Is it possible that a value overwrite an existing ID's value unintentionally?

Is it possible that a value overwrite an existing ID's value unintentionally?

Not sure that I follow.

This initially seems like a codesmell/poor design choice to me.

How else can you avoid duplicating logic that is the same regardless of the type of unbonding operation. For example, the CCV module is dealing with the completion of undelegation and redelegation in the same way. By adding different hooks and different IDs (e.g., ubdeID and redeID), CCV will need to duplicate code.

You use a dedicated key for each stored value type. Having to type check/error check based on the value is a codesmell. I.e. "I'm storing value X at key K. What is X? Idk, let me try these 5 different types..."

Got a second opinion form @ethanfrey and he tends to agree this approach is a code smell. We should refactor this. Would consider this a blocker for the merge. @mpoke any chance you or someone on your team can pick this up. once completed we can merge this work.

@marbar3778 @alexanderbez I will take a look at this right now. I wrote the offending code

The pattern in question is most clearly demonstrated not here, but in the top level code that calls this function in UnbondingCanComplete. UnbondingCanComplete works sort of like an if/else statement.

if (is redelegation) { } else if (is undelegation) { } else if (is validator unbonding) { } else { }

This didn't seem so bad when I wrote it, but I guess the part that troubles you is just the fact that references to records of different types are being kept in one store? Or is it that we are checking which type it is with failed deserializations?

The simplest way I can think of to avoid the latter issue is to make another index which stores unbondingId->type. Then we would check this first to find out what to deserialize to without the potential several failed deserializations which are bugging you.

but I guess the part that troubles you is just the fact that references to records of different types are being kept in one store?

Yes, exactly! It's that a key, a dedicated prefix really, can store values of different types yielding the need to have custom business logic of these if/else blocks as you pointed out. To me this is a code smell and no other SDK module does this.

What if we just combined/flattened all the possible value types into one type and have a field/enum that describes what kind of action to take?

something like

type MyValue struct { Type string // or enum // all fields that can represent everything you need }

Seems like maybe this is something a proto oneof could deal with efficiently

+	if !red.Entries[i].OnHold() && red.Entries[i].IsMature(ctx.BlockHeader().Time) {
+		// If matured, complete it.
+		// Remove entry
+		red.RemoveEntry(int64(i))


+		}
+
+		// Remove entry
+		ubd.RemoveEntry(int64(i))


+
+	// Convert into bytes for storage
+	bz := make([]byte, 8)
+	binary.BigEndian.PutUint64(bz, uint64(unbondingType))


julienrbrt · 2022-09-29T10:57:01Z

I have rebased to main here. I'll fix the tests and the build in #13420.

codecov · 2022-10-04T20:29:35Z

Codecov Report

Merging #13122 (a2e80d5) into main (6efbedb) will increase coverage by 0.03%.
The diff coverage is 67.38%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13122      +/-   ##
==========================================
+ Coverage   54.01%   54.04%   +0.03%     
==========================================
  Files         648      646       -2     
  Lines       55429    55711     +282     
==========================================
+ Hits        29938    30109     +171     
- Misses      23093    23188      +95     
- Partials     2398     2414      +16

Impacted Files	Coverage Δ
x/distribution/keeper/hooks.go	`22.50% <0.00%> (-0.58%)`	⬇️
x/evidence/keeper/infraction.go	`0.00% <0.00%> (ø)`
x/slashing/keeper/hooks.go	`51.11% <0.00%> (-2.38%)`	⬇️
x/slashing/keeper/infractions.go	`0.00% <0.00%> (ø)`
x/slashing/keeper/keeper.go	`52.94% <0.00%> (ø)`
x/staking/keeper/grpc_query.go	`1.74% <0.00%> (-0.01%)`	⬇️
x/staking/types/expected_keepers.go	`0.00% <ø> (ø)`
x/staking/types/hooks.go	`0.00% <0.00%> (ø)`
x/staking/types/keys.go	`42.42% <0.00%> (-1.79%)`	⬇️
x/staking/keeper/keeper.go	`61.40% <33.33%> (-7.49%)`	⬇️
... and 23 more

facundomedica

LGTM, added a couple of comments

I approved this without fully understanding the code, removing my approval to avoid any confusion

danwt · 2022-10-05T15:50:48Z

Just reminding that we please don't merge until we test this as a dependency in interchain-security/. Thanks.

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Co-authored-by: Simon <sainoe@users.noreply.github.com>

github-actions Bot added C:Simulations C:x/distribution distribution module related C:x/slashing C:x/staking labels Sep 1, 2022

tac0turtle mentioned this pull request Sep 1, 2022

WIP: Upstream to evaluate Liquid staking #12917

Closed

19 tasks

github-actions Bot added the C:x/evidence label Sep 6, 2022

tac0turtle marked this pull request as ready for review September 6, 2022 11:22

tac0turtle requested review from a team, alexanderbez and mpoke September 6, 2022 11:23

alexanderbez reviewed Sep 6, 2022

View reviewed changes

This was referenced Sep 8, 2022

Fork modified cosmos-sdk modules and add them to this repo cosmos/interchain-security#68

Closed

Make sure staking module changes work with Liquid Staking cosmos/interchain-security#67

Closed

jtremback reviewed Sep 22, 2022

View reviewed changes

Comment thread x/staking/keeper/unbonding_test.go Outdated

julienrbrt self-assigned this Sep 27, 2022

feat: IS rebase (squash da53124..b45dd5f)

83260b0

julienrbrt force-pushed the marko/IS branch from b45dd5f to 83260b0 Compare September 29, 2022 10:01

clean up docs

fa25727

github-advanced-security AI found potential problems Sep 29, 2022

View reviewed changes

julienrbrt mentioned this pull request Sep 29, 2022

fix: fix IS tests #13420

Merged

21 tasks

julienrbrt added 4 commits October 2, 2022 21:11

Merge branch 'main' into marko/IS

c6ccc62

fix: fix IS tests (#13420)

a6e5fe2

Merge branch 'main' into marko/IS

e6df12f

revert key update

944c3f6

julienrbrt added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Oct 4, 2022

julienrbrt changed the title ~~feat: IS rebase~~ feat!: IS rebase Oct 4, 2022

github-advanced-security AI found potential problems Oct 4, 2022

View reviewed changes

Comment thread x/staking/keeper/val_state_change.go Fixed

Comment thread x/staking/keeper/delegation.go Fixed

Comment thread x/staking/keeper/delegation.go Fixed

add hook error handling

eddaaf3

julienrbrt added 2 commits October 4, 2022 23:28

impl feedback

3092a51

add changelog

ad41ab4

julienrbrt reviewed Oct 4, 2022

View reviewed changes

Comment thread x/slashing/keeper/infractions.go

facundomedica previously approved these changes Oct 4, 2022

View reviewed changes

Comment thread tests/integration/staking/keeper/unbonding_test.go

Comment thread x/staking/keeper/slash.go

jtremback reviewed Oct 5, 2022

View reviewed changes

Comment thread x/staking/keeper/unbonding_test.go

Merge branch 'main' into marko/IS

49eb2b7

alexanderbez suggested changes Oct 7, 2022

View reviewed changes

julienrbrt added 5 commits October 7, 2022 16:11

implements style nits

4bef026

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

more nits

e53aa43

fix tests

ad79181

Co-authored-by: Simon <sainoe@users.noreply.github.com>

add godoc

caa6904

add godoc

8e86c27

julienrbrt approved these changes Oct 8, 2022

View reviewed changes

alexanderbez approved these changes Oct 8, 2022

View reviewed changes

Merge branch 'main' into marko/IS

a2e80d5

tac0turtle merged commit 84675a6 into main Oct 9, 2022

tac0turtle deleted the marko/IS branch October 9, 2022 10:21

sainoe mentioned this pull request Oct 12, 2022

Remove StakingHooksTemplate from provider hooks cosmos/interchain-security#389

Merged

julienrbrt mentioned this pull request Nov 28, 2022

refactor: remove gogoproto stringer annotations #14046

Merged

19 tasks

julienrbrt mentioned this pull request Apr 3, 2023

FEAT: ics #15663

Closed

This was referenced Apr 21, 2023

feat: update slashing and evidence module for ICS #15907

Closed

feat: update the slashing and evidence modules to work with ICS #15908

Merged

mpoke mentioned this pull request Jun 18, 2024

[Feature]: Remove unbonding pausing logic #20715

Closed

Conversation

tac0turtle commented Sep 1, 2022

Description

Author Checklist

Reviewers Checklist

Uh oh!

alexanderbez left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Check failure

Check failure

Check failure

julienrbrt commented Sep 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

facundomedica left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danwt commented Oct 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

codecov Bot commented Oct 4, 2022 •

edited

Loading