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

chore: remove UNFORKINGTODOs #8592

Merged
merged 3 commits into from
Aug 8, 2024
Merged

chore: remove UNFORKINGTODOs #8592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
985a6ed
chore: remove UNFORKINGTODOs
PaddyMc Aug 5, 2024
478afc7
chore: fix e2e tests
PaddyMc Aug 6, 2024
6073ab3
chore: fix tx command
PaddyMc Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions ante/sendblock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,6 @@ func (decorator *SendBlockDecorator) CheckIfBlocked(msgs []sdk.Msg) error {
return nil
}
for _, msg := range msgs {
// UNFORKING v2 TODO: GetSigners is no longer available
// This is the workaround for I did for all calls, verify it is correct.
signers, _, err := decorator.cdc.GetMsgV1Signers(msg)
if err != nil {
return err
Expand Down
13 changes: 2 additions & 11 deletions app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,9 +476,7 @@ func NewOsmosisApp(
// NOTE: staking module is required if HistoricalEntries param > 0
// NOTE: capability module's beginblocker must come before any modules using capabilities (e.g. IBC)

// UNFORKING v2 TODO: https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#set-preblocker
// The upgrading doc says we need to add upgrade types to pre blocker (done here), but also says we
// need to remove it from begin blocker. If we need to actually remove it, we need to change the SetOrderBeginBlockers logic.
// Upgrades from v0.50.x onwards happen in pre block
app.mm.SetOrderPreBlockers(upgradetypes.ModuleName)

// Tell the app's module manager how to set the order of BeginBlockers, which are run at the beginning of every block.
Expand All @@ -497,22 +495,15 @@ func NewOsmosisApp(
panic(err)
}

// UNFORKING v2 TODO: Verify that the NewBasicManagerFromManager call is correct.
// Notice I have to override the gov ModuleBasic with all the custom proposal handers, otherwise we lose them in the CLI.
// Override the gov ModuleBasic with all the custom proposal handers, otherwise we lose them in the CLI.
app.ModuleBasics = module.NewBasicManagerFromManager(
app.mm,
map[string]module.AppModuleBasic{
"gov": gov.NewAppModuleBasic(
[]govclient.ProposalHandler{
paramsclient.ProposalHandler,
// UNFORKING v2 TODO: Verify it is okay to remove these
// upgradeclient.LegacyProposalHandler,
// upgradeclient.LegacyCancelProposalHandler,
poolincentivesclient.UpdatePoolIncentivesHandler,
poolincentivesclient.ReplacePoolIncentivesHandler,
// UNFORKING v2 TODO: Verify it is okay to remove these
// ibcclientclient.UpdateClientProposalHandler,
// ibcclientclient.UpgradeProposalHandler,
superfluidclient.SetSuperfluidAssetsProposalHandler,
superfluidclient.RemoveSuperfluidAssetsProposalHandler,
superfluidclient.UpdateUnpoolWhitelistProposalHandler,
Expand Down
2 changes: 0 additions & 2 deletions app/encoding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ func MakeEncodingConfig() params.EncodingConfig {
encodingConfig := params.MakeEncodingConfig()
std.RegisterLegacyAminoCodec(encodingConfig.Amino)
std.RegisterInterfaces(encodingConfig.InterfaceRegistry)
// UNFORKING v2 TODO: Verify that we no longer need to register legacy amino codec
// keepers.AppModuleBasics.RegisterLegacyAminoCodec(encodingConfig.Amino)
keepers.AppModuleBasics.RegisterInterfaces(encodingConfig.InterfaceRegistry)
return encodingConfig
}
2 changes: 1 addition & 1 deletion app/modules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Did it say to do this somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 😕

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

e2e was failing on the cli not this

ord.FirstElements(epochstypes.ModuleName, capabilitytypes.ModuleName)

// Staking ordering
// TODO: Perhaps this can be relaxed, left to future work to analyze.
Expand Down
3 changes: 1 addition & 2 deletions cmd/osmosisd/cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,6 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) {

initRootCmd(rootCmd, encodingConfig, tempApp)

// UNFORKING v2 TODO: I don't think we have an option but to implement this. With out, the sdk queries do not show up in the CLI.
if err := autoCliOpts(initClientCtx, tempApp).EnhanceRootCommand(rootCmd); err != nil {
panic(err)
}
Expand Down Expand Up @@ -924,8 +923,8 @@ 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.
Copy link
Member

Choose a reason for hiding this comment

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

Did you confirm the legacy proposal sub commands still show?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

λ 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah soz, yeah they're missing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you find these commands in the v25.x release line? 😕

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This also makes e2e fail 😢

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

It fails because e2e uses these legacy commands, I have an issue to move away from legacy but alas no time

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#8599

I created this issue to track, should we merge this and deal with that issue in a vacuum

moduleBasics.AddTxCommands(cmd)

cmd.PersistentFlags().String(flags.FlagChainID, "", "The network chain ID")

return cmd
Expand Down
2 changes: 0 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,6 @@ replace (
// Needs to be replaced due to iavlFastNodeModuleWhitelist feature
cosmossdk.io/store => github.com/osmosis-labs/cosmos-sdk/store v0.1.0-alpha.1.0.20240509221435-b8feb2ffb728

// UNFORKING v2 TODO: No longer use wasmd fork, added logic to app.toml override to use 1000 instead of default 100 cache size, which was the reason for having the fork in the first place.

// Using branch osmo/v0.38.x
// https://github.com/osmosis-labs/cometbft/releases/tag/v0.37.4-v25-osmo-2
github.com/cometbft/cometbft => github.com/osmosis-labs/cometbft v0.0.0-20240510005818-6ce422c6f3d3
Expand Down