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

refactor cosmos init #8060

Merged
merged 7 commits into from
Jul 20, 2023
Merged

refactor cosmos init #8060

merged 7 commits into from
Jul 20, 2023

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Jul 18, 2023

Best reviewed commit-by-commit

Description

In order to support the migrations needed for #8025 #8031, and to implement #6527, we need to extend the cases where the snapshot logic is exercised. This PR simply adds guards where they were previously implicit through usage. It also fixes a potential bootstrap timing issue, where it was performed before the x/lien module was initialized. Finally it makes the timing of AG_COSMOS_INIT more explicit (no longer lazy on first BlockingSend)

Security Considerations

Adds checks regarding assumed init ordering

Scaling Considerations

N/A

Documentation Considerations

N/A

Testing Considerations

As with most logic straddling between golang and JS, there is little tests beyond the integration tests. This PR is changing little behavior, and what is changes does not have existing unit test coverage.

@mhofman mhofman requested review from michaelfig and JimLarson July 18, 2023 20:15
golang/cosmos/app/app.go Outdated Show resolved Hide resolved
golang/cosmos/app/app.go Show resolved Hide resolved
golang/cosmos/app/app.go Show resolved Hide resolved
golang/cosmos/app/app.go Show resolved Hide resolved
golang/cosmos/x/swingset/genesis.go Show resolved Hide resolved
golang/cosmos/x/swingset/keeper/snapshotter.go Outdated Show resolved Hide resolved
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

I've grokked what you're doing here. LGTM.

golang/cosmos/app/app.go Show resolved Hide resolved
golang/cosmos/app/app.go Outdated Show resolved Hide resolved
packages/cosmic-swingset/src/chain-main.js Show resolved Hide resolved
scripts/run-deployment-integration.sh Outdated Show resolved Hide resolved
@mhofman mhofman requested a review from JimLarson July 19, 2023 23:16
Copy link
Contributor

@JimLarson JimLarson left a comment

Choose a reason for hiding this comment

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

LGTM modulo two minor issues.

Comment on lines 1 to 5
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1


lastUpdateCheck 1689807326102
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this file get accidentally included?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yikes!

@@ -837,6 +838,7 @@ type cosmosInitAction struct {
// Name returns the name of the App
func (app *GaiaApp) Name() string { return app.BaseApp.Name() }

// CheckControllerInited exits if the controller initialization state does not match `expected`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The godoc tool doesn't parse markdown.

Suggested change
// CheckControllerInited exits if the controller initialization state does not match `expected`.
// CheckControllerInited exits if the controller initialization state does not match expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that a problem? In general I prefer delimiting code references with backticks even if they don't get parsed as markdown.

@mhofman mhofman force-pushed the mhofman/refactor-cosmos-init branch from 9aa87e3 to f19ebc8 Compare July 20, 2023 03:09
@mhofman mhofman force-pushed the mhofman/refactor-cosmos-init branch from f19ebc8 to 554a110 Compare July 20, 2023 04:04
@mhofman mhofman enabled auto-merge July 20, 2023 04:04
@mhofman mhofman added this pull request to the merge queue Jul 20, 2023
Merged via the queue into master with commit 22cbeb1 Jul 20, 2023
@mhofman mhofman deleted the mhofman/refactor-cosmos-init branch July 20, 2023 04:52
mhofman added a commit that referenced this pull request Aug 7, 2023
mhofman added a commit that referenced this pull request Aug 7, 2023
mhofman added a commit that referenced this pull request Aug 16, 2023
mhofman added a commit that referenced this pull request Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants