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

chore: audit x/upgrade module #2072

Merged
merged 10 commits into from
Aug 2, 2023
Merged

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jul 10, 2023

Closes #1961

@rootulp rootulp added the x/gov label Jul 10, 2023
@rootulp rootulp added this to the Mainnet milestone Jul 10, 2023
@rootulp rootulp self-assigned this Jul 10, 2023
@rootulp rootulp changed the title chore: audit x/gov module chore: audit x/upgrade module Jul 12, 2023
@rootulp rootulp marked this pull request as ready for review July 21, 2023 21:29
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2023

Codecov Report

Merging #2072 (a5d0302) into main (b77ac63) will increase coverage by 0.12%.
Report is 17 commits behind head on main.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main    #2072      +/-   ##
==========================================
+ Coverage   24.28%   24.41%   +0.12%     
==========================================
  Files         125      126       +1     
  Lines       14248    14300      +52     
==========================================
+ Hits         3460     3491      +31     
- Misses      10432    10448      +16     
- Partials      356      361       +5     
Files Changed Coverage Δ
x/blob/types/payforblob.go 69.54% <0.00%> (-4.02%) ⬇️
x/mint/types/constants.go 100.00% <ø> (ø)
test/txsim/blob.go 80.00% <100.00%> (-0.65%) ⬇️
x/blob/ante/ante.go 85.00% <100.00%> (-3.47%) ⬇️
x/blob/keeper/keeper.go 80.00% <100.00%> (-2.76%) ⬇️

... and 5 files with indirect coverage changes

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

Why add the x/upgrade module to our state machine? What behavior do we want to use from the upstream x/upgrade module?

Does the abstract answer this question? I can add portions of #1562 here as well if that would help. We don't want to be able to vote on upgrades, but we do still need some functionality so that IBC continue to compile/work. We also want to use the migration functionality, and we could even utilize the the halting/switching binaries mechanism as well should those things be needed.

x/upgrade/test/integration_test.go Outdated Show resolved Hide resolved
x/upgrade/README.md Outdated Show resolved Hide resolved
x/upgrade/README.md Outdated Show resolved Hide resolved
@MSevey MSevey requested a review from a team July 25, 2023 18:14
@rootulp rootulp marked this pull request as draft July 25, 2023 18:30
@rootulp
Copy link
Collaborator Author

rootulp commented Jul 26, 2023

Does the abstract answer this question?

IMO it didn't so I tried to elaborate on it in this PR. ADR-016 contains some more useful context so I copied that in.

but we do still need some functionality so that IBC continue to compile/work

It's not immediately clear what functionality is preserved in the forked x/upgrade module that is needed for IBC to work. Maybe we clarify that in a follow-up.

@rootulp rootulp marked this pull request as ready for review July 26, 2023 19:45
@rootulp
Copy link
Collaborator Author

rootulp commented Jul 26, 2023

Does our fork intentionally not register the CLI command? I can't do

$ ./build/celestia-appd query upgrade
Error: unknown command "upgrade" for "query"

Ref: https://docs.cosmos.network/v0.46/modules/upgrade/04_client.html#cli

also can't use tx commands

$ ./build/celestia-appd tx upgrade
Error: unknown command "upgrade" for "tx"

https://github.com/cosmos/cosmos-sdk/blob/v0.46.14/x/upgrade/client/cli/tx.go

@evan-forbes
Copy link
Member

evan-forbes commented Jul 27, 2023

It's not immediately clear what functionality is preserved in the forked x/upgrade module that is needed for IBC to work. Maybe we clarify that in a follow-up.

ref #1629 (comment)

unfortunately we are unable to compile using the ibc module since it relies on some upgrade module types

@rootulp rootulp removed the x/gov label Jul 27, 2023
@rootulp rootulp requested review from evan-forbes and removed request for evan-forbes July 30, 2023 14:31
@evan-forbes
Copy link
Member

re: the cli commands

since we are unable to submit the software upgrade proposals, imo we should also remove the cli commands that create them since there is no reason to create them

@rootulp rootulp merged commit 492c3db into celestiaorg:main Aug 2, 2023
20 checks passed
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.

Audit x/upgrade
4 participants