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: add init logic of module accounts just in case #1277

Merged
merged 8 commits into from
Mar 17, 2024

Conversation

jaeseung-bae
Copy link
Contributor

@jaeseung-bae jaeseung-bae commented Mar 12, 2024

Description

  • (audit issue) add init logic of foundation module accounts to InitGenesis in order to eliminate potential panic
  • NOTE: This is likely to change account number state in auth module. --> InitGenesis never runs more than once

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.74%. Comparing base (47a5e9f) to head (feb9962).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1277   +/-   ##
=======================================
  Coverage   69.74%   69.74%           
=======================================
  Files         643      643           
  Lines       67261    67266    +5     
=======================================
+ Hits        46909    46914    +5     
  Misses      18179    18179           
  Partials     2173     2173           
Files Coverage Δ
x/foundation/keeper/internal/genesis.go 97.40% <100.00%> (+0.18%) ⬆️

@jaeseung-bae jaeseung-bae self-assigned this Mar 12, 2024
@jaeseung-bae jaeseung-bae added A: bug Something isn't working C:x/foundation x/foundation module backport/v0.48.x labels Mar 12, 2024
@jaeseung-bae jaeseung-bae marked this pull request as ready for review March 12, 2024 12:39
@jaeseung-bae jaeseung-bae added A: State Machine Breaking Any changes that result in a different AppState given same genesisState and txList. and removed backport/v0.48.x labels Mar 13, 2024
@jaeseung-bae jaeseung-bae marked this pull request as draft March 13, 2024 10:50
@jaeseung-bae jaeseung-bae marked this pull request as ready for review March 15, 2024 02:55
@ulbqb
Copy link
Member

ulbqb commented Mar 15, 2024

NOTE: This is likely to change account number state in auth module.

You mean, this may have a negative impact on mainnet?

@jaeseung-bae
Copy link
Contributor Author

jaeseung-bae commented Mar 15, 2024

NOTE: This is likely to change account number state in auth module.

You mean, this may have a negative impact on mainnet?

A newly joined node that tries to sync with our operating node can never sync, because they may have different account number state.
So i labeled state breaking change, we need to apply this to the next major version. -> (note, InitGenesis runs only once)

GetModuleAccount will create a new account if not exists.

ulbqb
ulbqb previously approved these changes Mar 15, 2024
x/foundation/keeper/internal/genesis.go Outdated Show resolved Hide resolved
0Tech
0Tech previously approved these changes Mar 15, 2024
0Tech
0Tech previously approved these changes Mar 15, 2024
0Tech
0Tech previously approved these changes Mar 15, 2024
@jaeseung-bae jaeseung-bae added backport/v0.48.x and removed A: State Machine Breaking Any changes that result in a different AppState given same genesisState and txList. labels Mar 15, 2024
@0Tech 0Tech self-requested a review March 15, 2024 11:39
Copy link
Collaborator

@0Tech 0Tech left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ulbqb ulbqb left a comment

Choose a reason for hiding this comment

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

LGTM

@jaeseung-bae jaeseung-bae merged commit 089aff8 into Finschia:main Mar 17, 2024
38 checks passed
@jaeseung-bae jaeseung-bae deleted the fix/foundation-module-acc branch March 17, 2024 03:46
mergify bot pushed a commit that referenced this pull request Mar 17, 2024
* fix: add init logic of module accounts just in case

* chore: update changelog

* chore: check if module account successfully created

* chore: fix lint

* chore: add test

* chore: fix test

(cherry picked from commit 089aff8)

# Conflicts:
#	CHANGELOG.md
#	x/foundation/keeper/internal/genesis_test.go
jaeseung-bae added a commit that referenced this pull request Mar 19, 2024
…1283)

* fix: add init logic of module accounts just in case (#1277)

* fix: add init logic of module accounts just in case

* chore: update changelog

* chore: check if module account successfully created

* chore: fix lint

* chore: add test

* chore: fix test

(cherry picked from commit 089aff8)

# Conflicts:
#	CHANGELOG.md
#	x/foundation/keeper/internal/genesis_test.go

* Apply suggestions from code review

---------

Co-authored-by: jaeseung-bae <119839167+jaeseung-bae@users.noreply.github.com>
Co-authored-by: Youngtaek Yoon <noreply@yoon.mailer.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: bug Something isn't working backport/v0.48.x C:x/foundation x/foundation module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants