Invoke send_asset from call and exec

[Dominik1999]

We are assuming that send_asset will always be invoked via a call instruction. This should be the case most of the time, but it may also be invoked via an exec instruction if called from the account context. In such a case, the above code would result in an error.

So, I wonder if a better solution is to modify tx::create_note to return [ptr, 0, 0, 0, 0, 0, 0, 0, 0] rather than just [ptr] (would actually make tx::create_note simpler). @frisitano - what do you think?

Also, noticed that there is a small documentation error here - the return type should be [ptr, 0, 0, 0, 0, 0, 0, 0, 0], I believe.

Originally posted by @bobbinth in #233 (comment)

[frisitano]

We are assuming that send_asset will always be invoked via a call instruction. This should be the case most of the time, but it may also be invoked via an exec instruction if called from the account context. In such a case, the above code would result in an error.

So, I wonder if a better solution is to modify tx::create_note to return [ptr, 0, 0, 0, 0, 0, 0, 0, 0] rather than just [ptr] (would actually make tx::create_note simpler). @frisitano - what do you think?

I would be hesitant to support diverging from the standard of wrapping the kernel methods and dropping the padding for just tx::create_note. This inconsistency could result in poor dev-ex as they would not always be certain of the padding management that the user facing kernel procedures use. Would we also support this for other procedures as well in the future? It seems like this change would result in concerns of the wallet implementation leaking into the kernel implementation. Having said that, documenting this well should mitigate concerns around diverging from standard for the most part.

I wonder if an alternative would be to extract common code into a procedure (proc.do_send_asset) which would then subsequently be inlined as required. Any additional stack manipulation required can be handled by the caller procedure.

Also, noticed that there is a small documentation error here - the return type should be [ptr, 0, 0, 0, 0, 0, 0, 0, 0], I believe.

Good catch.

[frisitano]

Addressed documentation issue in #235

[bobbinth]

I generally agree that we shouldn't make one-off exceptions and try to follow the general approach as much as possible. But I think there are some considerations here worth thinking through more.

I'm somewhat hesitant to have procedures which work correctly only when invoked in specific contexts (especially, if these are procedures users may want to interact with). There are two ways to address this:

1. Make sure a procedure works correctly in every context.
2. Add some kind of checks to the assembler to make sure a procedure is not called in a wrong context.

Approach 1

If we go with the first approach, we could even adopt a convention that all procedures exported from an account module should be callable without issues both with call and exec instructions. And I also think it is fine if we pay a small performance penalty for enabling this.

Without changing the interface of tx::create_note, we could implement send_asset like so:

export.send_asset.1
    exec.account::remove_asset
    # => [ASSET, tag, RECIPIENT, ...]

    # insert 8 ZEROs into the stack right after recipient; we temporarily store one of the
    # elements of ASSET in memory to make stack manipulation easier
    loc_store.0 padw padw swapdw loc_load.0
    # => [ASSET, tag, RECIPIENT, ZERO, ZERO, ...]

    exec.tx::create_note
    # => [note_ptr, ZERO, ZERO, ...]
end

This should work and I think the extra performance cost is negligible.

I am a little worried, however, about the extra complexity we encounter here for just using tx::create_note procedure. Conceivably, if we are running into this complexity, others who may want to use it, will too. In general, we'll run into similar awkwardness whenever a procedure returns fewer stack items than parameters it received. But I'm having a hard time coming up with a general rule here.

Approach 2

To support the second approach, we'd need to introduce a new feature in the assembler - either some kind of procedure attribute. For example, something like:

@context(call)
export.send_asset
  ...
end

Then, if send_asset is invoked via an exec or syscall, the assembler would throw an error.

This type of a feature requires much more thinking, and is probably still a bit more brittle than making sure that a procedure can work in any context, but it could be also very useful for other purposes (e.g., describing shapes of parameters for auto-generation of Rust wrappers).

[Dominik1999]

Nice, I went to Approach 1. One thing to note:

I had to get rid of the 17th elements, so I added a swap drop at the end. Is this expected?

[bobbinth]

Superseded by #685.