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

feat: Add remaining node types for wasm #476

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

fl0rek
Copy link
Member

@fl0rek fl0rek commented Dec 12, 2024

No description provided.

@fl0rek fl0rek force-pushed the feat/wasm-types-coverage branch from 143630b to a9b69cd Compare December 12, 2024 08:21
@fl0rek fl0rek marked this pull request as ready for review December 12, 2024 13:17
@fl0rek fl0rek requested review from oblique and zvolin and removed request for oblique December 12, 2024 16:00
/// ```
#[derive(Debug, Clone, PartialEq, Eq)]
#[cfg(all(feature = "wasm-bindgen", target_arch = "wasm32"))]
#[wasm_bindgen]
Copy link
Member

Choose a reason for hiding this comment

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

I think the main point of making it wasm bindgenable was to also expose some methods on it, also getters for jsoned fields. without that, this looks less usable than just a json we had before

types/src/extended_header.rs Outdated Show resolved Hide resolved
types/src/data_availability_header.rs Outdated Show resolved Hide resolved
types/src/extended_header.rs Outdated Show resolved Hide resolved
#[wasm_bindgen(skip)]
pub validator_set: ValidatorSet,
/// Header of the block data availability.
#[wasm_bindgen(skip)]
Copy link
Member

Choose a reason for hiding this comment

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

In the above code you had this as #[wasm_bindgen(getter_with_clone)], which one is the correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since DAH is our own struct, we could wasm_bindgen it, so getter with clone is correct here, I believe.

fl0rek and others added 5 commits December 13, 2024 17:21
Co-authored-by: Yiannis Marangos <[email protected]>
Signed-off-by: Mikołaj Florkiewicz <[email protected]>
Co-authored-by: Yiannis Marangos <[email protected]>
Signed-off-by: Mikołaj Florkiewicz <[email protected]>
@fl0rek fl0rek force-pushed the feat/wasm-types-coverage branch from 8e27863 to 2abc466 Compare December 14, 2024 07:57
Comment on lines +29 to +30
const squareRows = networkHead.dah.row_roots().length;
const squareCols = networkHead.dah.column_roots().length;
Copy link
Member

@oblique oblique Dec 20, 2024

Choose a reason for hiding this comment

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

I'm not sure if I like this breaking change because JS it doesn't give even a runtime error if user does not migrate.

const squareRows = networkHead.dah.row_roots.length;
const squareCols = networkHead.dah.column_roots.length;

The above will actually give squareRows = 0 and squareCols = 0.

/// This function will also return an error if untrusted headers are not adjacent
/// to each other.
#[wasm_bindgen(js_name = verify_range)]
pub fn js_verify_range(&self, untrusted: Vec<ExtendedHeader>) -> Result<(), JsValue> {
Copy link
Member

@oblique oblique Dec 20, 2024

Choose a reason for hiding this comment

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

Because we move the value here, the JS value will be zeroed. Which is not nice.

Consider this example:

let h1 = await window.node.requestHeaderByHeight(BigInt(3109600));
let hdrs = await window.node.requestVerifiedHeaders(h1, BigInt(2));

h1.verify_range(hdrs);
let height = hdrs[0].height(); // This will panic because the headers were moved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants