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

fix: Only apply v8 fork logic on osmosis-1 #1610

Merged
merged 1 commit into from
May 27, 2022
Merged

Conversation

joeabbey
Copy link
Contributor

Closes: #1609

What is the purpose of the change

The v8 fork contained parameter changes that were only applicable on the "osmosis-1" chainID.

Brief Changelog

Limiting the RunForkLogic to osmosis-1

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

@joeabbey joeabbey requested a review from a team May 27, 2022 18:13
@codecov-commenter
Copy link

Codecov Report

Merging #1610 (a6e34a2) into main (2d8b9bd) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1610      +/-   ##
==========================================
- Coverage   19.60%   19.60%   -0.01%     
==========================================
  Files         242      242              
  Lines       32277    32279       +2     
==========================================
  Hits         6329     6329              
- Misses      24790    24792       +2     
  Partials     1158     1158              
Impacted Files Coverage Δ
app/upgrades/v8/forks.go 0.00% <0.00%> (ø)

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 2d8b9bd...a6e34a2. Read the comment docs.

@andrewl33
Copy link

Would a test that runs through all upgrades catch regressions like this going forward?

The tests could be fast if upgrade heights were in sequential.
It would mean these values would have to be overridable though, which might've been done before.

@ValarDragon
Copy link
Member

I think what should be done going forward is we add a flag / genesis file value for whether a given fork is following the Osmosis mainnet-only upgrades (things that refer to specific pool ID's, or incentive adjustments, etc.)

@ValarDragon ValarDragon added the A:backport/v8.x backport patches to v8.x branch label May 27, 2022
@ValarDragon ValarDragon merged commit e9062fc into main May 27, 2022
@ValarDragon ValarDragon deleted the joeabbey/fix-1609 branch May 27, 2022 18:49
mergify bot pushed a commit that referenced this pull request May 27, 2022
(cherry picked from commit e9062fc)

# Conflicts:
#	app/upgrades/v8/forks.go
ValarDragon added a commit that referenced this pull request May 29, 2022
* fix: Only apply v8 fork logic on osmosis-1 (#1610)

(cherry picked from commit e9062fc)

# Conflicts:
#	app/upgrades/v8/forks.go

* Fix merge conflict

Co-authored-by: Joe Abbey <[email protected]>
Co-authored-by: Dev Ojha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v8.x backport patches to v8.x branch
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Panic on RegisterWhitelistedDirectUnbondPools (on non-mainnet)
4 participants