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

Make token factory handle empty genesis configurations better #1551

Merged
merged 3 commits into from
May 20, 2022
Merged

Make token factory handle empty genesis configurations better #1551

merged 3 commits into from
May 20, 2022

Conversation

ValarDragon
Copy link
Member

What is the purpose of the change

A problem @czarcas7ic ran into was getting panics on token factory messages, when the genesis wasn't initialized.

Brief Changelog

  • Improve nil handling of token factory InitGenesis

Testing and Verifying

  • TODO

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? yes
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no, still pre-release for token factory
  • How is the feature or change documented? not applicable, removes a thing to document tbh

@ValarDragon ValarDragon requested a review from a team May 20, 2022 16:06
@ValarDragon
Copy link
Member Author

e2e test failure seems like some sort of docker issue?

@czarcas7ic
Copy link
Member

The local compilation is not hitting blocks, so essentially the update failed. I will go through it locally to see what the specific issue is for this pr

@codecov-commenter
Copy link

codecov-commenter commented May 20, 2022 •
edited
Loading

Codecov Report

Merging #1551 (b7ea7c1) into main (ba0c337) will increase coverage by 0.00%.
The diff coverage is 60.00%.

@@           Coverage Diff           @@
##             main    #1551   +/-   ##
=======================================
  Coverage   19.46%   19.47%           
=======================================
  Files         242      242           
  Lines       32255    32258    +3     
=======================================
+ Hits         6279     6282    +3     
  Misses      24822    24822           
  Partials     1154     1154
Impacted Files Coverage Δ
x/tokenfactory/keeper/createdenom.go 77.77% <33.33%> (+0.85%) ⬆️
x/tokenfactory/genesis.go 70.37% <100.00%> (+2.37%) ⬆️

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 ba0c337...b7ea7c1. Read the comment docs.

@czarcas7ic czarcas7ic merged commit d610ff3 into main May 20, 2022
@czarcas7ic czarcas7ic deleted the dev/validate_tokenfactory_genesis branch May 20, 2022 18:25
@czarcas7ic czarcas7ic added the A:backport/v9.x Do not use. backport patches to v9.x branch label May 20, 2022
mergify bot pushed a commit that referenced this pull request May 20, 2022
@ValarDragon @mergify
Make token factory handle empty genesis configurations better (#1551)
fff4cc6 
* Make token factory handle empty genesis configurations better

* add start back to dockerfile

Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
(cherry picked from commit d610ff3)

# Conflicts:
#	Dockerfile
@mergify mergify bot mentioned this pull request May 20, 2022
Merged
alexanderbez
alexanderbez reviewed May 20, 2022
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Curious why it'd ever be nil to begin with. Does default genesis state not use sdk.NewCoins?

@czarcas7ic
Copy link
Member

I ran into it when modifying the v8 genesis on LocalOsmosis to work on v9. Will the average user ever run into this case? Probably not. But its just a nice check to have since if it is null (like it was in my case) and it still gets through init genesis, it causes untraceable errors.

czarcas7ic added a commit that referenced this pull request May 20, 2022
@mergify @ValarDragon @czarcas7ic
Make token factory handle empty genesis configurations better (backport
46cc167 
#1551) (#1553)

* Make token factory handle empty genesis configurations better (#1551)

* Make token factory handle empty genesis configurations better

* add start back to dockerfile

Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
(cherry picked from commit d610ff3)

# Conflicts:
#	Dockerfile

* Update Dockerfile

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
@alexanderbez
Copy link
Contributor

We should add safe guards, I agree @czarcas7ic 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderbez alexanderbez alexanderbez left review comments

@czarcas7ic czarcas7ic czarcas7ic approved these changes

Assignees
No one assigned
Labels
A:backport/v9.x Do not use. backport patches to v9.x branch
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ValarDragon @czarcas7ic @codecov-commenter @alexanderbez