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.

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

Add minor comment for superfluid reward distr #1104

Merged
merged 9 commits into from
Mar 30, 2022

Conversation

mattverse
Copy link
Member

Description

Cref: #1070 (comment)


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@mattverse
Copy link
Member Author

@ValarDragon Fixed in addition to couple fixes!

@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2022

Codecov Report

Merging #1104 (418dd22) into main (271485d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1104   +/-   ##
=======================================
  Coverage   20.70%   20.70%           
=======================================
  Files         194      194           
  Lines       25259    25259           
=======================================
  Hits         5231     5231           
  Misses      19069    19069           
  Partials      959      959           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 271485d...418dd22. Read the comment docs.

@mattverse mattverse requested a review from ValarDragon March 28, 2022 06:47
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM once linter passes.
We may need to do var valAddr []byte, and remove the ineffassign , or add a nolint

x/superfluid/keeper/hooks_test.go Outdated Show resolved Hide resolved
app/apptesting/test_suite.go Outdated Show resolved Hide resolved
}

func (keeperTestHelper *KeeperTestHelper) BeginNewBlockWithProposer(executeNextEpoch bool, proposer sdk.ValAddress) {
valAddr := []byte(":^) at this distribution workaround")
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the context behind this, just curious?

Copy link
Member

@ValarDragon ValarDragon Mar 28, 2022

Choose a reason for hiding this comment

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

oh it was referring to it being hard to simply simulate an app.go begin block for tests (thats what all this function is doing) It was a super late night for superfluid staking, trying to get this test working lol, we should probably delete the message.

e.g. this + the last commit info was really confusing to get working (distribution would panic w/o a clear message if it wasn't present)

valAddr := []byte(":^) at this distribution workaround") //nolint
	validators := keeperTestHelper.App.StakingKeeper.GetAllValidators(keeperTestHelper.Ctx)
	if len(validators) >= 1 {
		valAddrFancy, err := validators[0].GetConsAddr()
		keeperTestHelper.Require().NoError(err)
		valAddr = valAddrFancy.Bytes()
	} else {
		valAddrFancy := keeperTestHelper.SetupValidator(stakingtypes.Bonded)
		validator, _ := keeperTestHelper.App.StakingKeeper.GetValidator(keeperTestHelper.Ctx, valAddrFancy)
		valAddr2, _ := validator.GetConsAddr()
		valAddr = valAddr2.Bytes()
	}valAddr := []byte(":^) at this distribution workaround") //nolint
	validators := keeperTestHelper.App.StakingKeeper.GetAllValidators(keeperTestHelper.Ctx)
	if len(validators) >= 1 {
		valAddrFancy, err := validators[0].GetConsAddr()
		keeperTestHelper.Require().NoError(err)
		valAddr = valAddrFancy.Bytes()
	} else {
		valAddrFancy := keeperTestHelper.SetupValidator(stakingtypes.Bonded)
		validator, _ := keeperTestHelper.App.StakingKeeper.GetValidator(keeperTestHelper.Ctx, valAddrFancy)
		valAddr2, _ := validator.GetConsAddr()
		valAddr = valAddr2.Bytes()
	}

app/apptesting/test_suite.go Outdated Show resolved Hide resolved
@ValarDragon ValarDragon merged commit 86a37ac into main Mar 30, 2022
@ValarDragon ValarDragon deleted the mattverse/superfluid-reward-distr-comments branch March 30, 2022 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants