x/chainmanager: Init types and params #22
Conversation
Init buf for proto management
…n and remove legacy subspace
…es and unused import
Raneet10
requested review from
lasarojc,
glnro,
sergio-mena,
marcello33,
pratikspatil024,
VAIBHAVJINDAL3012 and
a team
x/chainmanager/module.go
Outdated
|
|
||
| // TODO HV2: uncomment when keeper is implemented. We probably don't need invariants | ||
| // RegisterInvariants registers the chainmanager module invariants. | ||
| // func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) { |
There was a problem hiding this comment.
Yeah I'm not sure this will be necessary.
There was a problem hiding this comment.
Cool, will remove this then.
x/chainmanager/module.go
Outdated
|
|
||
| // TODO HV2: might not be needed since Route() was previously used in side handler routing | ||
| // Route returns the message routing key for the chainmanager module. | ||
| // func (AppModule) Route() string { |
x/chainmanager/module.go
Outdated
| // TODO HV2: uncomment when keeper is implemented | ||
| // InitGenesis performs genesis initialization for the chainmanager module. It returns | ||
| // no validator updates. | ||
| // func (am AppModule) InitGenesis(ctx sdk.Context, cdc codec.JSONCodec, data json.RawMessage) { |
There was a problem hiding this comment.
Uncomment, see above re: fulfilling expected interfaces
There was a problem hiding this comment.
Yeah this is uncommented in #23.
x/chainmanager/module.go
Outdated
|
|
||
| // TODO HV2: this is a no-op in current heimdall. Probably no need to implement this | ||
| // RegisterStoreDecoder registers a decoder for chainmanager module's types | ||
| // func (am AppModule) RegisterStoreDecoder(sdr simtypes.StoreDecoderRegistry) { |
x/chainmanager/module.go
Outdated
| // TODO HV2: this is a no-op in current heimdall. Probably no need to implement this | ||
| // looks equivalent to https://github.com/maticnetwork/heimdall/blob/249aa798c2f23c533d2421f2101127c11684c76e/chainmanager/module.go#L161C18-L161C34 | ||
| // ProposalMsgs returns msgs used for governance proposals for simulations. | ||
| // func (AppModule) ProposalMsgs(simState module.SimulationState) []simtypes.WeightedProposalMsg { |
There was a problem hiding this comment.
This is a gov module method for simulations, you don't need this in chain manager
There was a problem hiding this comment.
Got it, will remove in the simulation PR.
x/chainmanager/types/codec.go
Outdated
| // RegisterLegacyAminoCodec registers the necessary x/chainmanager interfaces and concrete types | ||
| // on the provided LegacyAmino codec. These types are used for Amino JSON serialization. | ||
| func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) { | ||
| cdc.RegisterConcrete(&Params{}, "heimdall-v2/x/bank/Params", nil) |
There was a problem hiding this comment.
Are there any others that could be included/executed here? I'm curious why a different module's type is registered here...
There was a problem hiding this comment.
Ahh that's a typo, my bad.
…yRoutes no-op + fix TODO comments
x/chainmanager: add CLI commands
| // error for any failed validation criteria. | ||
| func (gs GenesisState) Validate() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Should we add the validation here as now our addresses are string?
There was a problem hiding this comment.
Not sure what you meant here. We originally don't really do any validation.
| ) | ||
|
|
||
| var ( | ||
| ParamsKey = collections.NewPrefix(0) // ParamsKey is the key to store the params in the store |
There was a problem hiding this comment.
What is the major difference between heimdall approach of simply defining the key and by using collection?
There was a problem hiding this comment.
I actually second the usage of collections, I like its concept (see this @VAIBHAVJINDAL3012)
However: do we need to align any other module/PR?
x/chainmanager/module.go
Outdated
| return AppModule{ | ||
| // keeper: keeper, | ||
| // contractCaller: contractCaller, | ||
| legacySubspace: ss, |
There was a problem hiding this comment.
Can we also remove it?
Why have you commented out the keeper?
And please remove contractCaller comment also as in chainmanager it doesn't hold any meaning.
| @echo "Available targets:" | ||
| @echo " lint-deps - Install dependencies for GolangCI-Lint tool." | ||
| @echo " lint - Runs the GolangCI-Lint tool on the codebase." | ||
|
|
There was a problem hiding this comment.
This is all imported from @pratikspatil024 PR using the comos-sdk approach, right?
| @@ -1,41 +1,52 @@ | |||
| module github.com/0xPolygon/heimdall-v2 | |||
|
|
|||
| go 1.21.4 | |||
| go 1.21 | |||
There was a problem hiding this comment.
Any reason for downgrade? I'd actually bump it up to 1.22, but can be addressed afterwards ofc
|
|
||
| import "gogoproto/gogo.proto"; | ||
|
|
||
| option go_package = "github.com/0xPolygon/heimdall-v2/x/chainmanager/types"; |
There was a problem hiding this comment.
Maybe we could go with v1 @Raneet10 ? We'd need consistency across all modules/PRs but I believe we can proceed as any refactoring afterwards will be more error prone.
If agreed, let's inform the others too 🙏
| // This file is meant to track developer tools as dependencies in a go module | ||
| // See: https://github.com/go-modules-by-example/index/blob/master/010_tools/README.md | ||
|
|
||
| package tools |
There was a problem hiding this comment.
Do we really need this? I'd keep it minimal if not really relevant for us
| _ appmodule.AppModule = AppModule{} | ||
| ) | ||
|
|
||
| // TODO HV2: what should this value be ? |
There was a problem hiding this comment.
I think we don't need the ConsensusVersion at all, right @glnro ?
|
|
||
| // ValidateGenesis performs basic validation of chainmanager genesis data returning an | ||
| // error for any failed validation criteria. | ||
| func (gs GenesisState) Validate() error { |
There was a problem hiding this comment.
Do we need to validate anything here? Compared to heimdall I mean, where it does nothing
| return NewGenesisState(DefaultParams()) | ||
| } | ||
|
|
||
| // ValidateGenesis performs basic validation of chainmanager genesis data returning an |
| ) | ||
|
|
||
| var ( | ||
| ParamsKey = collections.NewPrefix(0) // ParamsKey is the key to store the params in the store |
There was a problem hiding this comment.
I actually second the usage of collections, I like its concept (see this @VAIBHAVJINDAL3012)
However: do we need to align any other module/PR?
| ) | ||
|
|
||
| var ( | ||
| ParamsKey = collections.NewPrefix(0) // ParamsKey is the key to store the params in the store |
There was a problem hiding this comment.
Let's move the comment on top of the line (instead of next to it)?
| if !common.IsHexAddress(value) { | ||
| return fmt.Errorf("invalid address for value %s for %s in chain_params", value, key) | ||
| } | ||
| if value == "" { |
There was a problem hiding this comment.
Isn't this check done already by common.IsHexAddress?
There, we have len(s) == 2*AddressLength && isHex(s), so if I am not mistaken the first part of that check already validates the length, right?
There was a problem hiding this comment.
Yeah the first check should suffice. Will remove.
x/chainmanager: add simulation
x/chainmanager: Implement gRPC querier
x/chainmanager: Init keeper and genesis
Description
This PR initialises types and params for chainmanager module.
To compare changes with current heimdall code see:
Changes
Checklist
Testing