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.

Sign up for GitHub

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

Jump to bottom

Consolidate code between InitGenesis and CreateGauge #766

Merged
merged 6 commits into from
Jan 31, 2022
Merged

Consolidate code between InitGenesis and CreateGauge #766

merged 6 commits into from
Jan 31, 2022

Conversation

pwang00
Copy link
Collaborator

@pwang00 pwang00 commented Jan 19, 2022

Closes: #644

Description

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

@codecov-commenter
Copy link

codecov-commenter commented Jan 19, 2022 •
edited
Loading

Codecov Report

Merging #766 (519b435) into main (42996df) will decrease coverage by 0.03%.
The diff coverage is 26.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #766      +/-   ##
==========================================
- Coverage   20.03%   19.99%   -0.04%     
==========================================
  Files         189      189              
  Lines       24566    24553      -13     
==========================================
- Hits         4921     4909      -12     
+ Misses      18790    18788       -2     
- Partials      855      856       +1
Impacted Files Coverage Δ
x/incentives/keeper/gauge.go 55.97% <26.31%> (+1.28%) ⬆️
x/gamm/keeper/math.go 100.00% <0.00%> (ø)
x/pool-incentives/client/cli/tx.go 0.00% <0.00%> (ø)
x/txfees/keeper/feedecorator.go 75.00% <0.00%> (+2.90%) ⬆️

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 42996df...519b435. Read the comment docs.

mattverse
mattverse reviewed Jan 26, 2022
View reviewed changes
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

LGTM in general!
Just curious about the variable styling used and should be good to go!

mattverse
mattverse reviewed Jan 27, 2022
View reviewed changes
Copy link
Member

@mattverse mattverse left a comment •
edited
Loading

Choose a reason for hiding this comment

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

What do you think about CreateGauge actually calling SetGaugeWithRefKey to further consolidate? (since SetGaugeWithRefKey seems to have duplicate code and SetGaugeWithRefKey covers some of the parts that are done in CreateGauge)?

@ValarDragon
Copy link
Member

Yeah I think we should probably move towards that. To get this merged sooner, lets make that into a second issue?

ValarDragon
ValarDragon approved these changes Jan 31, 2022
View reviewed changes
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!

@ValarDragon ValarDragon merged commit a90b0b0 into main Jan 31, 2022
@ValarDragon ValarDragon deleted the pwang00/consolidate_codepaths_644 branch January 31, 2022 20:13
ValarDragon pushed a commit that referenced this pull request Feb 10, 2022
@pwang00 @mattverse @ValarDragon
Consolidate code between InitGenesis and CreateGauge (#766)
067405e 
* Consolidate codepaths between InitGenesis and CreateGauge

* Update CHANGELOG.md

* Fix lint errors

* Update function name

* Update x/incentives/keeper/gauge.go comments

Co-authored-by: Matt, Park <[email protected]>

* Fix capitalization of variable names

Co-authored-by: Matt, Park <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattverse mattverse mattverse left review comments

@ValarDragon ValarDragon ValarDragon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consolidate code between incentives InitGenesis and CreateGauge
4 participants
@pwang00 @codecov-commenter @ValarDragon @mattverse