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

new crates: svm-types and svm-codec #153

Merged
merged 72 commits into from Jun 29, 2020
Merged

new crates: svm-types and svm-codec #153

merged 72 commits into from Jun 29, 2020

Conversation

YaronWittenstein
Copy link
Contributor

@YaronWittenstein YaronWittenstein commented Jun 14, 2020

Motivation

We're breaking the svm-app crate that logically contains:

  • Many common types and transaction types.
  • Encoding & decoding of SVM transactions

This PR will introduce two new crates:

  • svm-types: will contain the most types declared under svm-app.
  • svm-codec: will contain the encoding & decoding of transactions.
    This crate should also compile to a WASM file and be used by the Spacemesh Wallet
    (as part of synthesizing binary transactions to be submitted to the network).

Future PRs:

  • Depercating svm-app crate (using svm-codec instead).
    Other code of svm-app will move to the svm-runtime crate.

  • A likely future code change after this PR will be moving the code of the svm-common crate to sit under svm-types

@YaronWittenstein YaronWittenstein self-assigned this Jun 14, 2020
@YaronWittenstein YaronWittenstein requested a review from moshababo Jun 14, 2020
@YaronWittenstein YaronWittenstein added big Big-sized task enhancement New feature or request labels Jun 14, 2020
@YaronWittenstein YaronWittenstein marked this pull request as ready for review Jun 24, 2020
crates/svm-codec/src/template/wire.rs Show resolved Hide resolved
./../build.sh
cp ../../../target/wasm32-unknown-unknown/debug/svm_codec.wasm svm_codec.wasm

npm test
Copy link
Member

@moshababo moshababo Jun 27, 2020

Choose a reason for hiding this comment

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

You should add npm i. Otherwise mocha need to be a global dependency.

crates/svm-codec/examples/package.json Outdated Show resolved Hide resolved

function wasmNewBuffer(instance, object) {
const objectStr = JSON.stringify(object);
const bytes = new TextEncoder('utf-8').encode(objectStr);
Copy link
Member

@moshababo moshababo Jun 27, 2020

Choose a reason for hiding this comment

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

All tests are failing for me locally with ReferenceError: TextEncoder is not defined.

Which Nodejs version do you use? it should be specified in package.json.
Mine:

Moshes-MacBook-Pro:examples moshababo$ node -v
v10.15.3

If this is problematic, maybe you should use the well-supported Buffer class instead (assuming this code is intended to run in Nodejs and not in a browser).

Copy link
Contributor Author

@YaronWittenstein YaronWittenstein Jun 28, 2020

Choose a reason for hiding this comment

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

❯ node -v
v12.6.0

Copy link
Contributor Author

@YaronWittenstein YaronWittenstein Jun 28, 2020

Choose a reason for hiding this comment

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

I'll add to package.json

/// If the encoding failed, the returned WASM buffer will contain a JSON with the error.
#[no_mangle]
#[cfg(target_arch = "wasm32")]
pub extern "C" fn wasm_deploy_template(buf_ptr: i32) -> i32 {
Copy link
Member

@moshababo moshababo Jun 27, 2020

Choose a reason for hiding this comment

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

Also here I found it confusing - we're not deploying a template here, but encoding an app template.
So maybe wasm_encode_app_template instead?

Same for the other types.

Copy link
Contributor Author

@YaronWittenstein YaronWittenstein Jun 29, 2020

Choose a reason for hiding this comment

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

./../build.sh
cp ../../../target/wasm32-unknown-unknown/debug/svm_codec.wasm svm_codec.wasm

npm test
Copy link
Member

@moshababo moshababo Jun 27, 2020

Choose a reason for hiding this comment

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

This test need to be added to the CI.

/// Encodes a `deploy-template` binary-transaction using that JSON value.
///
/// Returns a pointer to a new WASM buffer holding the encoded transaction.
/// If the encoding failed, the returned WASM buffer will contain a JSON with the error.
Copy link
Member

@moshababo moshababo Jun 27, 2020

Choose a reason for hiding this comment

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

From reading this, it's not clear how the OK/ERROR result type can be detected. So perhaps you should add a brief explanation about that.

@YaronWittenstein YaronWittenstein merged commit cbfff60 into master Jun 29, 2020
3 checks passed
@YaronWittenstein YaronWittenstein deleted the svm-tx-codec branch Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
big Big-sized task enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants