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

feat: integrate with v0.52.x (2/n) #4423

Merged
merged 8 commits into from
Nov 26, 2024
Merged

feat: integrate with v0.52.x (2/n) #4423

merged 8 commits into from
Nov 26, 2024

Conversation

julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Nov 22, 2024

Follow-up of #4289

The previous PR implemented the required changes for the app and the minimum changes for the modules.

  • Use environment
  • Delete AppModuleBasic
  • Split files in module.go / depinject.go
  • Simplify testing

The follow-up will do the two remaining tasks:

  • Use core services / delete sdk.Context
  • Use new simulation framework

While the modules are working fine currently for 0.52, this bring them to the latest recommendations. The changes allows to make modules simpler, and baseapp and v2 compatible.

@julienrbrt julienrbrt marked this pull request as ready for review November 22, 2024 14:00
@Pantani
Copy link
Collaborator

Pantani commented Nov 22, 2024

@julienrbrt the export genesis command is not working

[IGNITE] error exporting state: genesis export error in mars: collections: not found: key 'no_key' of type github.com/cosmos/gogoproto/mars.mars.v1.Bla

@Pantani
Copy link
Collaborator

Pantani commented Nov 22, 2024

besides the genesis export command, everything is ok for me

@Pantani
Copy link
Collaborator

Pantani commented Nov 22, 2024

The app/export.go file still has sdk.Context declaration and sdk.ValAddressFromBech32(addr) decode. Do you know if we have plans to cover the refactor later?

@Pantani
Copy link
Collaborator

Pantani commented Nov 22, 2024

I also got some unhandled errors not in this PR. In the cmd/chaind/cmd/testnet.go there is a block:

app.DistrKeeper.ValidatorHistoricalRewards.Set(ctx, collections.Join(sdk.ValAddress(validator), uint64(0)), distrtypes.NewValidatorHistoricalRewards(sdk.DecCoins{}, 1))
app.DistrKeeper.ValidatorCurrentRewards.Set(ctx, validator, distrtypes.NewValidatorCurrentRewards(sdk.DecCoins{}, 1))
app.DistrKeeper.ValidatorsAccumulatedCommission.Set(ctx, validator, distrtypes.InitialValidatorAccumulatedCommission())
app.DistrKeeper.ValidatorOutstandingRewards.Set(ctx, validator, distrtypes.ValidatorOutstandingRewards{Rewards: sdk.DecCoins{}})

and deprecated addresses decode in this file also

@Pantani
Copy link
Collaborator

Pantani commented Nov 22, 2024

Maybe we should avoid scaffolding NewWeightedProposalMsg method into the x/chain/module/simulation.go file

@Pantani
Copy link
Collaborator

Pantani commented Nov 22, 2024

we need refactor the FindAccount method from x/module/simulation/helpers.go

@Pantani
Copy link
Collaborator

Pantani commented Nov 22, 2024

wdyt remove the ErrSample from x/module/types/errors.go

@Pantani
Copy link
Collaborator

Pantani commented Nov 22, 2024

I tried to review the PR, but it was hard because there were many files and also too many plush file changes. So, I scaffolded a chain and some types so I could check all the codes and change, and these are all my suggestions; I think we don't need to solve everything in this PR. We can open issues to solve later and before the v29. wdyt?

@julienrbrt
Copy link
Member Author

The app/export.go file still has sdk.Context declaration and sdk.ValAddressFromBech32(addr) decode. Do you know if we have plans to cover the refactor later?

Right, the sdk.Context removal will be done later as said in the description, same for simulations as it will be completely rewritten:

image

@julienrbrt
Copy link
Member Author

I'll check about export, but the rest was indeed planned for a follow-up

@julienrbrt
Copy link
Member Author

good point on the error, made me remember we have errors/v2! Updated. The rest will be done in a follow up.

@julienrbrt julienrbrt merged commit 9be0fb6 into main Nov 26, 2024
41 of 45 checks passed
@julienrbrt julienrbrt deleted the julien/module-052 branch November 26, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants