Implement chain extensions builtin #1305
Conversation
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
src/emit/substrate/target.rs
Outdated
| let success = binary | ||
| .builder | ||
| .build_int_compare(IntPredicate::EQ, ret, i32_zero!(), "ret"); | ||
| binary.builder.build_conditional_branch(success, ok, done); |
There was a problem hiding this comment.
What is supposed to happen if the return value from the syscall is not 0?
There was a problem hiding this comment.
However now that I think of it, it might be better to just trap. One could come up with the idea to try-catch it, but according to the solidity docs this is only supposed to be used for external calls
There was a problem hiding this comment.
Actually I don't know the reasoning behind that logic in instruction.rs. Is it guaranteed that we always stay in call or deploy and this return there will return from the contract? I'm not sure about this. I think we should just trap.
There was a problem hiding this comment.
I changed the behavior entirely. We should not trap or do anything on a non-zero return value. Instead, we should return this value back to the caller.
In the case of call_chain_extension, the return value being non-zero has no apparent meaning. It depends on the chain extensions concrete implementation. Hence we should just return it to the caller, as he should know how to handle this.
src/emit/substrate/target.rs
Outdated
| binary.builder.build_unconditional_branch(done); | ||
|
|
||
| binary.builder.position_at_end(done); | ||
| ret.into() |
There was a problem hiding this comment.
This looks wrong. Why is the syscall return value returned and not the vector?
There was a problem hiding this comment.
Are you sure? An int value is expected, a pointer won't work. I guessed that this int value is supposed to be the return code from the builtin.
The return value goes into the last argument to the builtin. Right above in this section, I store it there in the ok branch. I'm quite confident that I read the code correctly and implemented this builtin in the target accordingly. I mean the unit test passes, and out of curiosity I tried it out on a real substrate node with the fetch-rand chain extension example. It worked as expected.
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
docs/language/builtins.rst
Outdated
| Instead, it's specific to each chain extension. Hence, before using this builtin, | ||
| you must make sure that the chain extension being called is compatible. |
There was a problem hiding this comment.
How about replacing
Hence, before using this builtin, you must make sure that the chain extension being called is compatible.
with
Hence, when using this builtin, ensure that the chain extension is used in the correct way.
There was a problem hiding this comment.
This suggests that the chain extension function / ID of interest can be used in various ways, and the caller can do something about that, doesn't it? But this is not the case. Either the implementation of the chain extension adheres to this, or it doesn't. There is nothing you can do about it (unless your are also the author of the parachain runtime, but this is a bit of a stretch).
My main goal of this warning is to make contract authors to reconsider whether they know what they are doing. Calling into a chain extension that you're not 100% certain about what it does is a really, really bad idea anyways.
However I changed the wording slightly based on your comment, to try and make it more clear.
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Co-authored-by: Lucas Steuernagel <38472950+LucasSte@users.noreply.github.com> Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Provide a builtin to call chain extensions. This is the building block for implementing more high level interfaces directly in Solidity.