Skip to content
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

Engine method call has no way to attach ETH #309

Closed
birchmd opened this issue Oct 15, 2021 · 3 comments · Fixed by #351
Closed

Engine method call has no way to attach ETH #309

birchmd opened this issue Oct 15, 2021 · 3 comments · Fixed by #351
Assignees
Labels
C-enhancement Category: New feature or request C-good-first-issue Category: Good for newcomers

Comments

@birchmd
Copy link
Member

birchmd commented Oct 15, 2021

The method call is supposed to be equivalent to the method submit, except allows interacting with the EVM from a NEAR account as opposed to an Aurora account. However call does not have any way to attach an ETH balance to the transaction which greatly limits its usefulness.

I propose we add a value field to FunctionCallArgs struct to address this issue.

Note: to preserve backwards compatibility we may wish to accept both a version of FunctionCallArgs with and without the new field; in the case it is not given it defaults to 0 (the current behaviour). This may or may not be possible since borsh is not a self-describing format. If there are currently no mainnet usages of call then maybe the breaking change is fine.

@birchmd birchmd added C-enhancement Category: New feature or request C-good-first-issue Category: Good for newcomers labels Oct 15, 2021
@andrcmdr andrcmdr self-assigned this Oct 18, 2021
@mfornet
Copy link
Contributor

mfornet commented Nov 9, 2021

We can have a custom borsh deserialiser that reads the first two fields, and if there is some extra data in the end it reads it as the value (attached eth), this way we preserve backwards compatibility.

@andrcmdr andrcmdr linked a pull request Nov 9, 2021 that will close this issue
@andrcmdr
Copy link
Contributor

andrcmdr commented Nov 9, 2021

Finally find a way to overcome orphaning rules difficulties and made a PR.

Here's the feature branches:

Hope everything will work fine in current CI configuration ('cause not sure about this, due to toolkit update).

This is my first PR, I was doing my best, so don't hit me hard. =) Anyway, I rely on your advises and support guys!

@andrcmdr
Copy link
Contributor

I was radically change and simplify logic (following KISS & DRY), using more straightforward and explicit way with relying on generic borsh deserialization (io.read_input_borsh() and current to_value() method). (Thanks to @birchmd for working on loose coupling decomposition of SDK methods and host functions to IO trait for standalone engine).

Seems like now it works more reliable, rather than when using impl Trait based type matching implementation, which was less obvious, more implicit, less readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: New feature or request C-good-first-issue Category: Good for newcomers
Projects
None yet
4 participants