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

u128/i128 support in FFI #2622

Closed
mversic opened this issue Aug 15, 2022 · 4 comments
Closed

u128/i128 support in FFI #2622

mversic opened this issue Aug 15, 2022 · 4 comments
Assignees
Labels
Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST

Comments

@mversic
Copy link
Contributor

mversic commented Aug 15, 2022

The implementation after #2625 is outright wrong. u128/i128 is not FFI safe and cannot be sent across without serialization. I propose it to be serialized as FfiTuple2(u64, u64) or [u64; 2]

@mversic mversic added iroha2-dev The re-implementation of a BFT hyperledger in RUST question Further information is requested Enhancement New feature or request Red labels Aug 15, 2022
@mversic
Copy link
Contributor Author

mversic commented Aug 16, 2022

here is the tracking issue.

As far as I see u128 can be serialized in the following ways:
1.

#[repr(C)]
struct U128(u64, u64);
#[repr(C)]    // Note that I haven't used `repr(transparent)`
struct U128([u64; 2])

Idk if there is any difference between the two. The only potential difference that I can think of is that of the calling convention

In 2. I have used repr(C) due to a concern raised here. Whether or not repr(transparent) can be used in this case depends on whether the calling conventions for array and struct match but this is not something that should be depended upon unless confirmed

@appetrosyan
Copy link
Contributor

There are many C compilers and even more compilers that assume C-ABI behaviour.

I would very much prefer the first option, unless there was a good reason to use the second syntax.

In a vacuum (i.e. without ABI considerations) the first syntax is more explicit about the fact that you should really destructure both and use both parts of the value. You also don't add the baggage of iter() and many other things which don't map onto u128's real usage. The only case where the latter syntax would have been preferred is if the type u256 existed and we also needed to wrap it.

@appetrosyan appetrosyan removed the Red label Sep 12, 2022
@mversic mversic added Bug Something isn't working and removed question Further information is requested labels Sep 29, 2022
@appetrosyan appetrosyan added the blocked this problem can't be fixed yet label Oct 5, 2022
@mversic mversic removed the blocked this problem can't be fixed yet label Oct 7, 2022
@AlexStroke AlexStroke added the Dev defect The defect was found at the development stage. Did not have an impact on users. label Nov 14, 2022
@AlexStroke AlexStroke self-assigned this Dec 5, 2022
@AlexStroke AlexStroke removed the Dev defect The defect was found at the development stage. Did not have an impact on users. label Dec 5, 2022
@AlexStroke
Copy link
Contributor

No code currently relies on this, but if it did it would be a bug.

@AlexStroke AlexStroke removed their assignment Dec 5, 2022
@AlexStroke AlexStroke added QA-confirmed This bug is reproduced and needs a fix Dev defect The defect was found at the development stage. Did not have an impact on users. labels Dec 5, 2022
@appetrosyan appetrosyan removed Dev defect The defect was found at the development stage. Did not have an impact on users. QA-confirmed This bug is reproduced and needs a fix labels Dec 15, 2022
@appetrosyan
Copy link
Contributor

Then it's really good that this is a WIP, rather than in production. If we're labelling hypotheticals as bugs, we should add the QA-confirmed tag to all issues.

@appetrosyan appetrosyan removed the Bug Something isn't working label Dec 15, 2022
@appetrosyan appetrosyan assigned Erigara and unassigned pesterev and QuentinI May 18, 2023
Erigara added a commit to Erigara/iroha that referenced this issue Jun 1, 2023
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 2, 2023
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
appetrosyan pushed a commit to Erigara/iroha that referenced this issue Jun 2, 2023
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
appetrosyan pushed a commit that referenced this issue Jun 2, 2023
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Erigara added a commit to Erigara/iroha that referenced this issue Jun 2, 2023
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
appetrosyan pushed a commit that referenced this issue Jun 5, 2023
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
mversic pushed a commit that referenced this issue Oct 17, 2023
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants