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

[feature] #2161: generate FFI functions for data_model #2164

Closed

Conversation

mversic
Copy link
Contributor

@mversic mversic commented Apr 28, 2022

Signed-off-by: Marin Veršić marin.versic101@gmail.com

Description of the Change

You can find a description here: #1973

This PR introduces an ffi crate which holds a macro ffi_bindgen. This macro when attached to the impl block generates the equivalent ffi out of the methods listed in the impl block. The macro can also be attached to the structure definition where it integrates with getset to make ffi fn equivalents of the getters/setters produced by the getset.

Note:

  1. All FFI functions return FfiResult return code. The idea is that the returned code can be used to query the error
  2. Considering the previous points, all FFI functions take, as a last argument, a mutable pointer where the return value of the corresponding method is stored (the so called out-pointer pattern)

Limitations:

  1. mutable references are not supported for input arguments, except for the self argument, i.e. handle

Issue

Closes #2161

Benefits

Possible Drawbacks

Usage Examples or Tests [optional]

#[derive(Getters)]
#[getset(get = "pub")]
struct Struct {
    elem: u32,
    #[getset(skip)]
    elems: Vec<u32>,
}

#[ffi_bindgen]
impl Struct {
    pub fn new(elem: u32) -> Self {
        Self {elem, elems: vec![]}
    }
    pub fn get_elems(&self) -> impl ExactSizeIterator<&u32> {
        self.elems.iter()
    }
}

fn main() {
    let s = struct_new(42);
    let elems = MaybeUninit::<*const *const u32>::uninit();
    let elems_len = MaybeUninit::<usize>::uninit();
    if FfiResult::Ok != unsafe {struct_elems(&s, elems.as_mut_ptr(), elems_len.as_mut_ptr())} {
        // TODO: Handle error
    }
    let elems_len = unsafe {elems_len.assume_init()};
    let elems = unsafe {elems.assume_init()};
}

Alternate Designs [optional]

List out ffi functions manually. This is tedious and highly error prone because we're operating in the unsafe space

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Apr 28, 2022
#[no_mangle]
pub unsafe extern "C" fn dbg(ptr: *const u8, len: usize) {
host::dbg(ptr, len)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the three functions above are just temporary placeholders. In the future smart contract will talk to host only through this module but there will be more meat in these functions. Namely, they'll convert opaque pointers received from the smart contract into scale encoded byte slice before passing them to the host for execution

@mversic mversic force-pushed the data_model_generate_ffi_fns branch from 9cfd519 to 8eff696 Compare April 28, 2022 16:40
range: RangeFrom<usize>,
) -> T {
let bytes = Box::from_raw(core::slice::from_raw_parts_mut(ptr as *mut _, len as usize));
let bytes = Box::from_raw(core::slice::from_raw_parts_mut(ptr as *mut _, len));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is potentially UB, though I think it's not

Copy link
Contributor

Choose a reason for hiding this comment

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

While I'd err on the side of caution, I think you're right in it not being intrinsically UB.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you really want to be pedantic, maybe add an assert, that checks for conversion errors.

@mversic mversic force-pushed the data_model_generate_ffi_fns branch 4 times, most recently from 4aeef40 to a749028 Compare April 29, 2022 18:32
range: RangeFrom<usize>,
) -> T {
let bytes = Box::from_raw(core::slice::from_raw_parts_mut(ptr as *mut _, len as usize));
let bytes = Box::from_raw(core::slice::from_raw_parts_mut(ptr as *mut _, len));
Copy link
Contributor

Choose a reason for hiding this comment

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

While I'd err on the side of caution, I think you're right in it not being intrinsically UB.

range: RangeFrom<usize>,
) -> T {
let bytes = Box::from_raw(core::slice::from_raw_parts_mut(ptr as *mut _, len as usize));
let bytes = Box::from_raw(core::slice::from_raw_parts_mut(ptr as *mut _, len));
Copy link
Contributor

Choose a reason for hiding this comment

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

If you really want to be pedantic, maybe add an assert, that checks for conversion errors.

core/wasm/Cargo.toml Show resolved Hide resolved
core/wasm/build.rs Show resolved Hide resolved

## Usage

Please follow the [WASM section of our tutorial](https://hyperledger.github.io/iroha-2-docs/guide/advanced/intro.html#wasm) for a detailed guide

## How to build smart contract?
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be synchronised to the docs repo.

@mversic mversic force-pushed the data_model_generate_ffi_fns branch from a749028 to 5150159 Compare May 2, 2022 10:23
@appetrosyan appetrosyan self-assigned this May 5, 2022
@mversic mversic force-pushed the data_model_generate_ffi_fns branch 2 times, most recently from 98c40fb to 48a27a3 Compare May 5, 2022 13:02
@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #2164 (48a27a3) into iroha2-dev (ea4f5e3) will increase coverage by 20.46%.
The diff coverage is 57.64%.

❗ Current head 48a27a3 differs from pull request most recent head f8d1fce. Consider uploading reports for the commit f8d1fce to get more accurate results

@@               Coverage Diff               @@
##           iroha2-dev    #2164       +/-   ##
===============================================
+ Coverage            0   20.46%   +20.46%     
===============================================
  Files               0      187      +187     
  Lines               0    26973    +26973     
===============================================
+ Hits                0     5519     +5519     
- Misses              0    21454    +21454     
Impacted Files Coverage Δ
cli/src/lib.rs 0.59% <0.00%> (ø)
client/examples/million_accounts_genesis.rs 0.00% <0.00%> (ø)
client/src/config.rs 69.35% <0.00%> (ø)
client/tests/integration/add_asset.rs 0.00% <0.00%> (ø)
client/tests/integration/asset_propagation.rs 0.00% <0.00%> (ø)
...lient/tests/integration/multiple_blocks_created.rs 0.00% <0.00%> (ø)
client/tests/integration/multisignature_account.rs 0.00% <0.00%> (ø)
...nt/tests/integration/multisignature_transaction.rs 0.00% <0.00%> (ø)
client/tests/integration/non_mintable.rs 0.00% <0.00%> (ø)
client/tests/integration/pagination.rs 0.00% <0.00%> (ø)
... and 212 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea4f5e3...f8d1fce. Read the comment docs.

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@mversic mversic force-pushed the data_model_generate_ffi_fns branch from 87f554a to 6de64fe Compare May 9, 2022 11:46
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@mversic mversic force-pushed the data_model_generate_ffi_fns branch from 6de64fe to 125862b Compare May 9, 2022 14:05
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@mversic mversic force-pushed the data_model_generate_ffi_fns branch from 125862b to f8d1fce Compare May 9, 2022 14:25
@appetrosyan appetrosyan deleted the branch hyperledger:iroha2-dev May 13, 2022 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants