feat: preinstalls #226
feat: preinstalls #226
Conversation
x/vm/types/preinstall.go
Outdated
| import "github.com/ethereum/go-ethereum/common" | ||
|
|
||
| type Preinstall struct { | ||
| Address common.Address |
There was a problem hiding this comment.
Could we add names here to keep track of what these are?
There was a problem hiding this comment.
Yeah, I think that is a good idea. I will be doing that!
x/vm/module.go
Outdated
| @@ -127,6 +127,11 @@ func (AppModule) Name() string { | |||
| func (am AppModule) RegisterServices(cfg module.Configurator) { | |||
| types.RegisterMsgServer(cfg.MsgServer(), am.keeper) | |||
| types.RegisterQueryServer(cfg.QueryServer(), am.keeper) | |||
|
|
|||
| m := keeper.NewMigrator(am.keeper) | |||
| if err := cfg.RegisterMigration(types.ModuleName, 9, m.Migrate9to10); err != nil { | |||
There was a problem hiding this comment.
Same as above - I want to avoid having migrations for modules before v1
x/vm/genesis.go
Outdated
| @@ -57,6 +57,10 @@ func InitGenesis( | |||
| } | |||
| } | |||
|
|
|||
| if err := k.AddPreinstalls(ctx, types.DefaultPreinstalls); err != nil { | |||
There was a problem hiding this comment.
Could we add this into the evmd instead? I'm thinking that we could pass in the default preinstalls as a default there, but allow anyone to specify their own precompiles upon genesis inside their app
There was a problem hiding this comment.
Yeah for sure, I also felt there was something slightly off about this, so I will take another look to make this more reasonable.
There was a problem hiding this comment.
I've made this change as well, let me know if it is what you had in mind.
I haven't really tested it yet, is there a good way to test evmd genesis that I should be doing beyond what I have added unit tests for?
|
I believe I've made the required changes, so putting this in ready-for-review. |
| // return errorsmod.Wrapf(types.ErrInvalidPreinstall, "preinstall %s already has an account in account keeper", preinstall.Address.String()) | ||
| // create account with the account keeper | ||
| account := k.accountKeeper.NewAccountWithAddress(ctx, accAddress) | ||
| k.accountKeeper.SetAccount(ctx, account) |
There was a problem hiding this comment.
Why do we not want to error when an account exists--like you have in the comment?
There was a problem hiding this comment.
Oh yeah, this was actually something I hadn't fully finished thinking through, but the initial thinking was that this would be problematic these already existed. After the changes, this should not really happen, I think.
I'll have another look at this part!
There was a problem hiding this comment.
It is also an option to check here if this already exists, and then also check if the given account and code all match, and then just ignore and continue. Not sure if that is a legit scenario though 🤔
There was a problem hiding this comment.
Oh yeah, I might have done this because all the integration tests fail with this. It seems like they don't reset the account keeper or something.
I'm a bit lost with this integration test suite. Any pointers from you or @vladjdk on this?
There was a problem hiding this comment.
Which tests? All integ tests are passing with this diff
| @@ -27,7 +27,7 @@ import ( | |||
| ) | |||
|
|
|||
| // consensusVersion defines the current x/evm module consensus version. | |||
| const consensusVersion = 9 | |||
| const consensusVersion = 10 | |||
There was a problem hiding this comment.
Let's leave this--I think we're going to reset this for the production release since I don't think anyone is actually running module version 9.
There was a problem hiding this comment.
I'm going to merge this and follow up on a couple of things in a separate PR.
Thanks @gjermundgaraba !
I want to discuss how we add this, and if we want any temporary stuff or how we should do this.
In the current code I have the following:
The questions I have are:
a. Eric mentioned maybe wanting a cli command to add pre-installs. I could do that, but it seems like temporary code we might now want in the evm repo
b. If this is primarily to add them to the running testnet, I would suggest just changing the gaia binary being used to call the `keeper.Add