-
Notifications
You must be signed in to change notification settings - Fork 355
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
Upgrade to 0.14.0 alpha2 #235
Conversation
I did some cleanup to see if this passes CI now. I wonder if we should rename |
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.
Looking good. If there are some particular tests you want me to look at closer, just add a comment there.
I am curious about renaming HandleMsg
-> ExecuteMsg
and anything else we need to change like that for consistency
@@ -176,7 +179,8 @@ pub fn handle_burn_from( | |||
Ok(meta) | |||
})?; | |||
|
|||
let res = HandleResponse { | |||
let res = Response { |
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.
Would it make sense to extend Response::default()
?
Or do something like:
let mut res = Response::default();
res.add_attribute("action", "burn_from");
res.add_attribute("from", owner);
res.add_attribute("by", deps.api.human_address(&spender_raw));
res.add_attribute("amount", amount);
Ok(res)
We could do this consistently if you like (at least on a few contracts as demo and make an issue to finish it).
NB: I would prefer the shorter (and more Ethereum like) res.emit("from", owner)
but Simon didn't like that as it wasn't clear. Curious as to your opinion.
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 prefer it as it is now, for clarity.
A default()
and overwriting is discouraged for style, by the way. See https://rust-lang.github.io/rust-clippy/master/#field_reassign_with_default
A default can still be useful, though now that addresses don't have defaults, a "full default" is not possible anymore.
I think the sensible thing would be to rename it. Particularly when writing new contracts, it would be confusing to have to write a Maybe we can leave this change for a future version, though, as not to introduce so many changes at once. |
We can do that in another PR, but let's do this by the next v0.6.0 cosmwasm-plus release, so that is a stable point. See #236 |
Only missing piece AFAIK, was migrating to the new entry point system. If the old system still works (I think so), we can leave that for the next iteration. |
Yeah, they both work and there is a new issue for that one. It's a bit down the backlog #230 But if you want to, you can pull that next (it shouldn't be a big deal) |
Closes #229 .
A couple of tests in multi-test are failing, but publishing anyway as they are interesting cases to look at (
wrongsender can now send bank messages in the multi-test context).Update: This makes sense, because
BankMsg
doesn't have afrom_address
anymore. Will just modify / remove these tests.Still needs some styling, and closely following
MIGRATING.md
guidelines, but this is compiling, passes linting AFAIK, and passesalmostall tests.