-
Notifications
You must be signed in to change notification settings - Fork 337
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
Merge messages submessages #961
Conversation
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 start. I wonder if this is really helping if it requres us to set ID, reply on and gas limit for every message.
I will fix a few compiler errors then leave this here for us to reflect on naming. It seems the only real issue is the best naming/syntax to convert |
Yes, but I think it is a conceptual question: how do we preserve the simplicity of messages if we integrate them into the fully featured submessages. |
That was my thinking to allow |
Making proposed changes from #953 (comment) One thing that I realized was let mut res = Response::new();
res.add_message(BankMsg::Send{...});
res.add_submessage(SubMsg::reply_on_error(WasmMsg::Execute{ .. }, 1234)); They both are pretty clear APIs. |
fcd9d58
to
58b755a
Compare
Good questions. Let'd do this for the first step and then see if/how we can improve from there. |
Just missing the MIGRATING.md updates, but waiting for final review and any last API changes for that. |
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.
Very cool.
Could you as part of this change also remove this?
impl Default for ReplyOn {
fn default() -> Self {
ReplyOn::Always
}
}
I don't think ReplyOn
should have a default anymore.
@@ -8,6 +8,7 @@ while getopts c option; do | |||
case "${option}" in | |||
|
|||
c) op="check" ;; | |||
*) ;; |
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.
Ah, @uint could you install shellcheck
on your system? It is executed automatically on all our scripts if available because of this line: command -v shellcheck >/dev/null && shellcheck "$0"
e3630b3
to
7813dc1
Compare
Okay, made requested changes. Let's see if the CI catches anything |
@webmaster128 from my view, this is done and ready to merge. Can I get a final approval, or last change requests? |
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 solution 🏅 Added some minor text polishing as a commit. Good to merge.
Closes #953
Closes #948
This is a start now to show a new API, I will update some contracts to give a feel how easy/hard this is
BankMsg::Send{}.into()
gives aSubMsg<T>
)submessages
field and makemessages
field useSubMsg
notCosmosMsg
std
vm
and other packages