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 v44 auth upgrade bug #618

Merged
merged 1 commit into from Nov 25, 2021
Merged

Fix v44 auth upgrade bug #618

merged 1 commit into from Nov 25, 2021

Conversation

ValarDragon
Copy link
Member

Workaround for cosmos/cosmos-sdk#10591 , https://github.com/provenance-io/provenance/blob/407c89a7d73854515894161e1526f9623a94c368/app/upgrades.go#L86-L122

Just do auth upgrade last.


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 Report

Merging #618 (3960798) into main (f68d6ed) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #618      +/-   ##
==========================================
- Coverage   18.44%   18.44%   -0.01%     
==========================================
  Files         172      172              
  Lines       24233    24235       +2     
==========================================
  Hits         4470     4470              
- Misses      18998    19000       +2     
  Partials      765      765              
Impacted Files Coverage Δ
app/app.go 0.55% <0.00%> (-0.01%) ⬇️

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 f68d6ed...3960798. Read the comment docs.

@robert-zaremba
Copy link
Contributor

This look as a dirty hack. I'm thinking about changing the map value type to a structure with priority.

@robert-zaremba
Copy link
Contributor

Let's continue the discussion in the SDK issue

@yaruwangway yaruwangway mentioned this pull request Nov 23, 2021
14 tasks
@ValarDragon
Copy link
Member Author

Yeah, but can't wait 2+ weeks for it to land tho

@ValarDragon
Copy link
Member Author

Seeing as this is being tested on vega upgrade as well, merging

@ValarDragon ValarDragon merged commit c0b9e9a into main Nov 25, 2021
@ValarDragon ValarDragon deleted the dev/fix_v44_auth_upgrade_bug branch November 25, 2021 05:23
@robert-zaremba
Copy link
Contributor

sure, was only saying that it's feels hacky, but solves the problem. In 0.44 we don't have other solution which is not breaking :/ except the patch for x/auth.

So your one is the best so far. I have created a draft PR for 0.45 to change the structure.

@ValarDragon
Copy link
Member Author

ValarDragon commented Nov 27, 2021

ah 100% agreed, its absolutely a gross workaround lol. Thanks for getting a better fix into 0.45

ValarDragon added a commit that referenced this pull request Dec 7, 2021
ValarDragon added a commit that referenced this pull request Dec 7, 2021
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.

None yet

3 participants