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

Host API #182

Merged
merged 40 commits into from Oct 25, 2020
Merged

Host API #182

merged 40 commits into from Oct 25, 2020

Conversation

YaronWittenstein
Copy link
Contributor

@YaronWittenstein YaronWittenstein commented Oct 19, 2020

Motivation

Ergonomic API to be used when implementing Apps in Rust (for example - in upcoming Vault app).

  • Removed the Host Context (mapping between host fields indexes to blobs),
    instead, the Host (i.e go-spacemesh) will inject these fields as imports to SVM.
    For example: if SVM running app will want to know its sender address
    it will call an imported function named: host_sender (under wasm namespace host).

  • Mock Host API implementation.

  • Testing the external Host API (ExtHost) will be done when testing the Vault app.

@YaronWittenstein YaronWittenstein self-assigned this Oct 19, 2020
@YaronWittenstein YaronWittenstein requested a review from moshababo Oct 19, 2020
@YaronWittenstein YaronWittenstein added the enhancement New feature or request label Oct 19, 2020
@YaronWittenstein YaronWittenstein changed the title svm-sdk Host API Host API Oct 19, 2020
@YaronWittenstein YaronWittenstein marked this pull request as ready for review Oct 22, 2020

/// ### `offset` meaning in this file:
///
/// The parameter `offset here denotes a memory address integer serving as a pointer to a cell to the current
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The closing backtick for offset is missing.


/// ### `offset` meaning in this file:
///
/// The parameter `offset here denotes a memory address integer serving as a pointer to a cell to the current
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"pointer to a cell to the current" -> "pointer to a cell within the current"?


/// Signals to SVM that the current running transaction output (a.k.a `returndata`)
/// lays out in memory starting from offset `offset` and its byte-length is `length`.
fn svm_returndata(offset: u32, length: u32);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you specify the expected behaviour in case the program fails to call it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


/// Sends to SVM the logging message that starts
/// at memory offset `offset` (of byte-length `length`)
/// and it's associated message code (for signaling errors severity such as `trace/info/error` etc.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the message codes / log levels values already defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to restrict it here.
I think that each app can come up with its own error codes.

///
/// If other blockchain projects will want to take SVM and use it for their purposes then they
/// should bring their own `host imports`.
#[link(wasm_import_module = "host")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you already defined the imports as Spacemesh-specific, perhaps also consider importing them as sm/spacemesh instead of host.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea


/// Signals to SVM that the current running transaction output (a.k.a `returndata`)
/// lays out in memory starting from offset `offset` and its byte-length is `length`.
fn svm_returndata(offset: u32, length: u32);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling it svm_set_returndata, same as in the Host trait, will make more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np (I thought it'll be nicer to keep it short but not critical)

/// Its methods delegate work to the singleton `InnerHost` instance
/// which also implements the `Host` trait and contains the actual implementation of the `Host` trait.
///
/// In order to get access to this singleton instance one the API user should use `ExtHost::instance()`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"singleton instance one the API" - redundant word?

@YaronWittenstein YaronWittenstein merged commit d5ba7a7 into master Oct 25, 2020
@YaronWittenstein YaronWittenstein deleted the svm-sdk-host branch Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants