-
Notifications
You must be signed in to change notification settings - Fork 404
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
Bump proto #563
Bump proto #563
Conversation
Codecov Report
@@ Coverage Diff @@
## master #563 +/- ##
==========================================
+ Coverage 59.61% 59.65% +0.03%
==========================================
Files 45 45
Lines 5260 5260
==========================================
+ Hits 3136 3138 +2
+ Misses 1893 1892 -1
+ Partials 231 230 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐎
// MigrateMsg json encoded message to be passed to the contract on migration | ||
bytes migrate_msg = 6; | ||
// Msg json encoded message to be passed to the contract on migration | ||
bytes msg = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice changes. I was wondering: why is there no ExecuteContractProposal? Would this be useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm.. what would it do?
Migrate will override any other auth.
InstantiateProposal is useful if we want to instantiate a code ID which is governance restricted (you can set it so only governance can create new contracts, to avoid eg. everyone making cw20 tokens on a non-DeFi chain)
What would the Execute Proposal do?
All I can think of is to execute a message as an arbitrary account. Which is as powerful as "allow governance to forge a message from arbitrary user". You can make an issue to describe this idea if you think it would be useful and let's wait for Alex to discuss it.
x/wasm/types/wasmer_engine.go
Outdated
@@ -82,7 +82,7 @@ type WasmerEngine interface { | |||
// replace it. This allows it to run a migration step if needed, or return an error if unable to migrate | |||
// the given data. | |||
// | |||
// MigrateMsg has some data on how to perform the migration. | |||
// Msg has some data on how to perform the migration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This propably refers to the argument migrateMsg
which is not renamed. Let's use the variable name here in text, no matter which one we use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over aggressive renaming.
Should I rename the variables in this interface as well? I guess it has nothing to do with proto and is non API-breaking, so we can do it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind. I don't even understand why this file exist and those signatures are not imported from wasmvm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interface that matches wasmvm (the implementation), but allows him to swap out the vm with a mock that let's us test mock contracts in Go. This makes for much simpler unit testing of the higher level logic (eg. go contract that returns such and such events in reply, then see how the are assembled through the keeper)
9f68533
to
d643241
Compare
Closes #558