chore: remove UNFORKINGTODOs #8592
Conversation
WalkthroughThe recent changes involve a comprehensive refactoring aimed at enhancing clarity, simplifying the codebase, and improving module management within the application. Key modifications include the removal of outdated comments, adjustments to module order during initialization, and streamlining transaction command handling. These adjustments not only refine the application's architecture but also contribute to improved maintainability and efficiency, all while preserving essential functionality. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
| @@ -221,7 +221,7 @@ func orderBeginBlockers(allModuleNames []string) []string { | |||
| // Epochs must come before staking, because txfees epoch hook sends fees to the auth "fee collector" | |||
| // module account, which is then distributed to stakers. If staking comes before epochs, then the | |||
| // funds will not be distributed to stakers as expected. | |||
| ord.FirstElements(upgradetypes.ModuleName, epochstypes.ModuleName, capabilitytypes.ModuleName) | |||
There was a problem hiding this comment.
Did it say to do this somewhere?
There was a problem hiding this comment.
https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#set-preblocker
Yeah it did, but it's failing e2e, I think we may need to keep it the way it is 😕
There was a problem hiding this comment.
e2e was failing on the cli not this
| @@ -924,8 +923,6 @@ func txCommand(moduleBasics module.BasicManager) *cobra.Command { | |||
| authcmd.GetDecodeCommand(), | |||
| ) | |||
|
|
|||
| // UNFORKING v2 TODO: Auto CLI claims we can remove this, but if we do, then the legacy proposal sub commands will not be available. | |||
There was a problem hiding this comment.
Did you confirm the legacy proposal sub commands still show?
There was a problem hiding this comment.
λ ghost osmosis → λ git chore/unforking-todos* → make install
Go version: 1.22
GOWORK=off go install -mod=readonly -tags "netgo ledger" -ldflags '-X github.com/cosmos/cosmos-sdk/version.Name=osmosis -X github.com/cosmos/cosmos-sdk/version.AppName=osmosisd -X github.com/cosmos/cosmos-sdk/version.Version=adam/sqs-v26-testing-68-g985a6ede8 -X github.com/cosmos/cosmos-sdk/version.Commit=985a6ede848f2cc1048ba92ddf180eee222ed637 -X "github.com/cosmos/cosmos-sdk/version.BuildTags=netgo,ledger" -w -s' -trimpath github.com/osmosis-labs/osmosis/v25/cmd/osmosisd
λ ghost osmosis → λ git chore/unforking-todos* → osmosisd tx gov submit-legacy-proposal --help
Submit a legacy proposal along with an initial deposit.
Proposal title, description, type and deposit can be given directly or through a proposal JSON file.
Example:
$ osmosisd tx gov submit-legacy-proposal --proposal="path/to/proposal.json" --from mykey
Was there another command that you noticed missing?
There was a problem hiding this comment.
ah soz, yeah they're missing
There was a problem hiding this comment.
Can you find these commands in the v25.x release line? 😕
There was a problem hiding this comment.
Did you confirm the legacy proposal sub commands still show?
I'm struggling here a little, can you add one that I can copy/pasta?
There was a problem hiding this comment.
This also makes e2e fail 😢
There was a problem hiding this comment.
Do osmosisd q gov submit-legacy-proposal -h and you will see these Available Commands either missing or present:
Available Commands:
cancel-software-upgrade Cancel the current software upgrade proposal
create-cl-pool-and-cfmm-link Submit a create clpool and link to cfmm proposal
create-concentratedliquidity-pool-proposal Submit a create concentrated liquidity pool proposal
create-groups-proposal Submit a create groups proposal
denom-pair-taker-fee-proposal Submit a denom pair taker fee proposal
ibc-upgrade Submit an IBC upgrade proposal
migrate-cw-pool-contracts Submit a migrate cw pool contracts proposal
param-change Submit a parameter change proposal
remove-superfluid-assets-proposal Submit a superfluid asset remove proposal
replace-migration-records-proposal Submit a replace migration record proposal
replace-pool-incentives Submit a full replacement to the records for pool incentives
set-scaling-factor-controller-proposal Submit a set scaling factor controller proposal
set-superfluid-assets-proposal Submit a superfluid asset set proposal
software-upgrade Submit a software upgrade proposal
tick-spacing-decrease-proposal Submit a tick spacing decrease proposal
update-client Submit an update IBC client proposal
update-fee-token Submit a update fee token record proposal
update-migration-records-proposal Submit a update migration record proposal
update-pool-incentives Submit an update to the records for pool incentives
update-unpool-whitelist Update unpool whitelist proposal
upload-code-id-and-whitelist Submit an upload code id and whitelist proposal
There was a problem hiding this comment.
It fails because e2e uses these legacy commands, I have an issue to move away from legacy but alas no time
There was a problem hiding this comment.
ah ye, they are missing, I've added them back now, the issue we have now is now some commands are doubled, but only the commands with:
λ ghost osmosis → λ git chore/unforking-todos → osmosisd tx | grep -
ibc-transfer IBC fungible token transfer transaction subcommands
ibc-transfer IBC fungible token transfer transaction subcommands
ibc-wasm IBC wasm manager module transaction subcommands
ibc-wasm IBC wasm manager module transaction subcommands
interchain-accounts IBC interchain accounts transaction subcommands
interchain-accounts IBC interchain accounts transaction subcommands
There was a problem hiding this comment.
I created this issue to track, should we merge this and deal with that issue in a vacuum
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
cmd/osmosisd/cmd/root.go (1)
Potential issue with legacy proposal subcommands
The
submit-legacy-proposalsubcommand is not found in theNewRootCmdfunction. This might indicate that it has been removed or was never implemented. Please verify if this is intentional or if it needs to be re-added.
- File:
cmd/osmosisd/cmd/root.go- Function:
NewRootCmdAnalysis chain
Line range hint
427-427: Confirm legacy proposal subcommands.Ensure that the legacy proposal subcommands are still available and functional after the removal of the UNFORKINGTODO comments.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the legacy proposal subcommands are available. # Test: Check the help output for legacy proposal subcommands. osmosisd tx gov submit-legacy-proposal --helpLength of output: 123
Script:
#!/bin/bash # Search for the definition of the NewRootCmd function in the cmd/osmosisd/cmd/root.go file ast-grep --lang go --pattern 'func NewRootCmd() $_' # Search for the registration of legacy proposal subcommands within the NewRootCmd function ast-grep --lang go --pattern '$_ := &cobra.Command{ Use: "tx", Short: "Transactions subcommands", $$$ } $_ := &cobra.Command{ Use: "gov", Short: "Governance subcommands", $$$ } $_ := &cobra.Command{ Use: "submit-legacy-proposal", Short: "Submit a legacy proposal", $$$ }'Length of output: 10457
Script:
#!/bin/bash # Search for the registration of the `tx` subcommand within the NewRootCmd function ast-grep --lang go --pattern '$_ := &cobra.Command{ Use: "tx", Short: "Transactions subcommands", $$$ }' cmd/osmosisd/cmd/root.go # Search for the registration of the `gov` subcommand within the NewRootCmd function ast-grep --lang go --pattern '$_ := &cobra.Command{ Use: "gov", Short: "Governance subcommands", $$$ }' cmd/osmosisd/cmd/root.go # Search for the registration of the `submit-legacy-proposal` subcommand within the NewRootCmd function ast-grep --lang go --pattern '$_ := &cobra.Command{ Use: "submit-legacy-proposal", Short: "Submit a legacy proposal", $$$ }' cmd/osmosisd/cmd/root.goLength of output: 36765
PaddyMc
force-pushed
the
chore/unforking-todos
branch
2 times, most recently
from
df639c4
to
aa936b3
Compare
PaddyMc
force-pushed
the
chore/unforking-todos
branch
from
aa936b3
to
6073ab3
Compare
PaddyMc
added
A:no-changelog
V:state/compatible/no_backport
What is the purpose of the change
In preparation for the v0.50 upgrade we left some UNFORKINGTODO around the code base to verify some functionality, this PR removes and an answers a few of those questions.