-
Notifications
You must be signed in to change notification settings - Fork 431
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
Unify ink_env::{eval_contract, invoke_contract}
#1165
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.
@@ -192,7 +171,7 @@ where | |||
/// .push_arg(true) | |||
/// .push_arg(&[0x10; 32]) | |||
/// ) | |||
/// .returns::<ReturnType<i32>>() | |||
/// .returns::<i32>() |
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.
❤️ that users aren't exposed to ReturnType<T>
anymore
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.
In the PR description you mention that "the user can either omit the call to returns::<R>()
". Doesn't looks like this is the case, if I remove this line the example fails to compile
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.
Oh it appears that is only the case if the arg list is not set (which in this example it is) see https://github.com/paritytech/ink/blob/c0cccb74270f4ef9b92ea3dbde7a06b25f1bf091/crates/env/src/call/call_builder.rs#L438-L439.
Here is an example which calls explcitily but with no args so that compiles: https://github.com/paritytech/ink/blob/378d83f3fc204e18dce6f3946e6a0f27e333d7d6/examples/proxy/lib.rs#L80
I'm not sure why it was already this way, it would make sense to either allow it to be Unset
either way and default to ()
, or require it to be set in all cases. What do you think?
CI fails due to the |
Unifies these two functions which already are both calling the same underlying function, except
invoke
passes()
as the return type parameter.If using the call builder API explicitly to invoke a message which does have a return value but you want to skip decoding it, as was the previous behaviour of
invoke_contract
, the user can either omit the call toreturns::<R>()
or explicitly setreturns::<()>()
. This will result as before in a no-op for the decoding so would be optimised away.Closes #814