R4R: Rename hooks from Before -> Pre and On -> Post #3076

jackzampolin · 2018-12-10T23:10:00Z

Fixes #2538

This PR renames the hooks in the SDK to match the agreed upon format in the linked issue.

cc @rigelrozanski @cwgoes

alexanderbez

As it's a breaking SDK API change, I think this warrants a pending log update, otherwise, LGTM.

cwgoes

These names aren't right, I think we want Pre/Post to reflect whether or not the store has been updated.

Also needs a PENDING.md update.

cwgoes · 2018-12-11T13:50:42Z

x/stake/handler.go

@@ -163,7 +163,7 @@ func handleMsgEditValidator(ctx sdk.Context, msg types.MsgEditValidator, k keepe
 			return err.Result()
 		}
 		validator.Commission = commission
-		k.OnValidatorModified(ctx, msg.ValidatorAddr)
+		k.PostValidatorModified(ctx, msg.ValidatorAddr)


This isn't post-modification? Changes haven't been written to the store yet (line 169).

cwgoes · 2018-12-11T13:51:01Z

x/stake/keeper/delegation.go

@@ -398,9 +398,9 @@ func (k Keeper) Delegate(ctx sdk.Context, delAddr sdk.AccAddress, bondAmt sdk.Co

 	// call the appropriate hook if present
 	if found {
-		k.OnDelegationSharesModified(ctx, delAddr, validator.OperatorAddr)
+		k.PostDelegationSharesModified(ctx, delAddr, validator.OperatorAddr)


This isn't post-modification.

All of the hooks were On something. I'll have to read through the code in a bit more detail to figure out which ones are Pre.

Yes, the previous names were (are) wrong / misleading

cwgoes · 2018-12-11T13:51:06Z

x/stake/keeper/delegation.go

 	} else {
-		k.OnDelegationCreated(ctx, delAddr, validator.OperatorAddr)
+		k.PostDelegationCreated(ctx, delAddr, validator.OperatorAddr)


This isn't post-modification either.

fedekunze · 2018-12-11T13:52:47Z

I only see On -> Post changes here. Maybe the others are missing ? Otherwise we need to change the title

cwgoes · 2018-12-12T12:19:14Z

x/stake/handler.go

@@ -122,7 +122,7 @@ func handleMsgCreateValidator(ctx sdk.Context, msg types.MsgCreateValidator, k k
 	k.SetValidatorByConsAddr(ctx, validator)
 	k.SetNewValidatorByPowerIndex(ctx, validator)

-	k.OnValidatorCreated(ctx, validator.OperatorAddr)
+	k.PostValidatorCreated(ctx, validator.OperatorAddr)


(this is post-modification, 👍 )

cwgoes · 2018-12-12T12:19:34Z

x/stake/keeper/delegation.go

@@ -81,7 +81,7 @@ func (k Keeper) SetDelegation(ctx sdk.Context, delegation types.Delegation) {

 // remove a delegation from store
 func (k Keeper) RemoveDelegation(ctx sdk.Context, delegation types.Delegation) {
-	k.OnDelegationRemoved(ctx, delegation.DelegatorAddr, delegation.ValidatorAddr)
+	k.PostDelegationRemoved(ctx, delegation.DelegatorAddr, delegation.ValidatorAddr)


This isn't post-modification (as you can see by the store.Delete call two lines below)

cwgoes · 2018-12-12T12:20:05Z

x/stake/keeper/delegation.go

@@ -431,7 +431,7 @@ func (k Keeper) unbond(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValA
 		return
 	}

-	k.OnDelegationSharesModified(ctx, delAddr, valAddr)
+	k.PostDelegationSharesModified(ctx, delAddr, valAddr)


This isn't post-modification (we set the delegation later in this function)

cwgoes · 2018-12-12T12:20:19Z

x/stake/keeper/val_state_change.go

@@ -81,7 +81,7 @@ func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx sdk.Context) (updates []ab
 			// Assert that the validator had updated its ValidatorDistInfo.FeePoolWithdrawalHeight.
 			// This hook is extremely useful, otherwise lazy accum bugs will be difficult to solve.
 			if k.hooks != nil {
-				k.hooks.OnValidatorPowerDidChange(ctx, validator.ConsAddress(), valAddr)
+				k.hooks.PostValidatorPowerDidChange(ctx, validator.ConsAddress(), valAddr)


this is post-modification, 👍

cwgoes · 2018-12-12T12:20:46Z

x/stake/keeper/val_state_change.go

@@ -197,7 +197,7 @@ func (k Keeper) bondValidator(ctx sdk.Context, validator types.Validator) types.

 	// call the bond hook if present
 	if k.hooks != nil {
-		k.hooks.OnValidatorBonded(ctx, validator.ConsAddress(), validator.OperatorAddr)
+		k.hooks.PostValidatorBonded(ctx, validator.ConsAddress(), validator.OperatorAddr)


(this is post-modification, 👍 )

cwgoes · 2018-12-12T12:20:59Z

x/stake/keeper/val_state_change.go

@@ -232,7 +232,7 @@ func (k Keeper) beginUnbondingValidator(ctx sdk.Context, validator types.Validat

 	// call the unbond hook if present
 	if k.hooks != nil {
-		k.hooks.OnValidatorBeginUnbonding(ctx, validator.ConsAddress(), validator.OperatorAddr)
+		k.hooks.PostValidatorBeginUnbonding(ctx, validator.ConsAddress(), validator.OperatorAddr)


(this is post-modification, 👍 )

cwgoes · 2018-12-12T12:21:14Z

x/stake/keeper/validator.go

@@ -204,7 +204,7 @@ func (k Keeper) RemoveValidator(ctx sdk.Context, address sdk.ValAddress) {

 	// call hook if present
 	if k.hooks != nil {
-		k.hooks.OnValidatorRemoved(ctx, validator.ConsAddress(), validator.OperatorAddr)
+		k.hooks.PostValidatorRemoved(ctx, validator.ConsAddress(), validator.OperatorAddr)


(this is post-modification, 👍 )

cwgoes

@jackzampolin I've done a thorough review and labeled which hooks should be which, I think (always good to oonfirm - just look at where the store.Set calls are in relation to the hook invocation).

codecov · 2018-12-12T23:36:58Z

Codecov Report

Merging #3076 into develop will increase coverage by <.01%.
The diff coverage is 50%.

@@             Coverage Diff             @@
##           develop    #3076      +/-   ##
===========================================
+ Coverage    54.85%   54.86%   +<.01%     
===========================================
  Files          133      133              
  Lines         9544     9546       +2     
===========================================
+ Hits          5235     5237       +2     
  Misses        3988     3988              
  Partials       321      321

jackzampolin · 2018-12-12T23:37:27Z

I've addressed the comments here and properly named the functions (according to @cwgoes). Would be good to get 👀 from @rigelrozanski here

cwgoes · 2018-12-13T00:16:26Z

Merge develop; that should fix the sim.

x/stake/genesis.go

jaekwon · 2018-12-19T22:01:32Z

Can we do "Before"/"After"? or "Will"/"Did". "Pre"/"Post" seems like they would be overloaded terms in general.

jackzampolin · 2018-12-19T22:10:39Z

@jaekwon Done. I think this is ready to go in after that.

cwgoes · 2018-12-21T17:34:31Z

LGTM, this needs another review still.

rigelrozanski

Generally looks good - still a few comments which I'd like addressed

x/distribution/keeper/hooks.go

x/slashing/hooks.go

x/stake/keeper/val_state_change.go

x/stake/handler.go

x/stake/genesis.go

Co-Authored-By: jackzampolin <[email protected]>

jackzampolin · 2019-01-07T20:52:24Z

Is this PR waiting on anything @cwgoes @rigelrozanski ? Looks like I've addressed the comments?

cwgoes · 2019-01-08T21:55:24Z

LGTM except linter fails.

jackzampolin · 2019-01-09T00:05:02Z

Turned off the linter on the affected file. This is ready to go and can be merged whenever.

rigelrozanski

@jackzampolin just for reference my comments where not fully addressed (I added leftover comments to another issue as well as made one commit) - Generally speaking, somebodies PR comments should only be considered "addressed" once all the the PR comments have been marked as resolved, which was not this case here (nbd obv. - just mentioning)

jackzampolin requested a review from rigelrozanski December 10, 2018 23:10

jackzampolin requested review from cwgoes and ebuchman as code owners December 10, 2018 23:10

jackzampolin changed the title ~~Rename hooks from Before -> Pre and On -> Post~~ R4R: Rename hooks from Before -> Pre and On -> Post Dec 10, 2018

alexanderbez reviewed Dec 11, 2018

View reviewed changes

cwgoes suggested changes Dec 11, 2018

View reviewed changes

cwgoes reviewed Dec 12, 2018

View reviewed changes

cwgoes suggested changes Dec 12, 2018

View reviewed changes

cwgoes reviewed Dec 13, 2018

View reviewed changes

x/stake/genesis.go Outdated Show resolved Hide resolved

jackzampolin force-pushed the jack/rename-hooks branch from ad19b42 to a8664c9 Compare December 13, 2018 00:19

cwgoes approved these changes Dec 13, 2018

View reviewed changes

jackzampolin mentioned this pull request Dec 17, 2018

Release 0.29 Tracking #3139

Closed

cwgoes mentioned this pull request Dec 18, 2018

"Red Wedding" (first GoS hard fork) master to-do list #3079

Closed

7 tasks

jackzampolin force-pushed the jack/rename-hooks branch from ebf2929 to fd1647f Compare December 19, 2018 22:04

Rename hooks from Before -> Pre and On -> Post

ffad6ae

jackzampolin force-pushed the jack/rename-hooks branch from cbdb264 to ffad6ae Compare December 20, 2018 19:11

Fix lint error

821d444

rigelrozanski added the ready-for-review label Dec 27, 2018

rigelrozanski suggested changes Jan 2, 2019

View reviewed changes

Update x/slashing/hooks.go

815a4eb

Co-Authored-By: jackzampolin <[email protected]>

Ignore linter warnings on hooks file

1711e79

rigelrozanski mentioned this pull request Jan 9, 2019

Deduplicate / DRY / cleanup hooks #3174

Closed

swap 171, 172 for clarity (my own comment)

ec0c932

rigelrozanski approved these changes Jan 9, 2019

View reviewed changes

rigelrozanski merged commit b16af44 into develop Jan 9, 2019

rigelrozanski deleted the jack/rename-hooks branch January 9, 2019 05:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Rename hooks from Before -> Pre and On -> Post #3076

R4R: Rename hooks from Before -> Pre and On -> Post #3076

jackzampolin commented Dec 10, 2018

alexanderbez left a comment

cwgoes left a comment

cwgoes Dec 11, 2018

cwgoes Dec 11, 2018

jackzampolin Dec 11, 2018

cwgoes Dec 11, 2018

cwgoes Dec 11, 2018

fedekunze commented Dec 11, 2018

cwgoes Dec 12, 2018

cwgoes Dec 12, 2018

cwgoes Dec 12, 2018

cwgoes Dec 12, 2018

cwgoes Dec 12, 2018

cwgoes Dec 12, 2018

cwgoes Dec 12, 2018

cwgoes left a comment

codecov bot commented Dec 12, 2018 •

edited

Loading

jackzampolin commented Dec 12, 2018

cwgoes commented Dec 13, 2018

jaekwon commented Dec 19, 2018

jackzampolin commented Dec 19, 2018

cwgoes commented Dec 21, 2018

rigelrozanski left a comment

jackzampolin commented Jan 7, 2019

cwgoes commented Jan 8, 2019

jackzampolin commented Jan 9, 2019

rigelrozanski left a comment •

edited

Loading

R4R: Rename hooks from Before -> Pre and On -> Post #3076

R4R: Rename hooks from Before -> Pre and On -> Post #3076

Conversation

jackzampolin commented Dec 10, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze commented Dec 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 12, 2018 • edited Loading

Codecov Report

jackzampolin commented Dec 12, 2018

cwgoes commented Dec 13, 2018

jaekwon commented Dec 19, 2018

jackzampolin commented Dec 19, 2018

cwgoes commented Dec 21, 2018

rigelrozanski left a comment

Choose a reason for hiding this comment

jackzampolin commented Jan 7, 2019

cwgoes commented Jan 8, 2019

jackzampolin commented Jan 9, 2019

rigelrozanski left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 12, 2018 •

edited

Loading

rigelrozanski left a comment •

edited

Loading