Skip to content

Add wasm macro to bindings crate wasm functions #1721

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

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

Conversation

findolor
Copy link
Collaborator

@findolor findolor commented May 1, 2025

Motivation

See issue: #1601 and #1619

Solution

  • Move wasm functions in bindings crate to js api crate
  • Implement wasm macro to each binding
  • Update tests

Checks

By submitting this for review, I'm confirming I've done the following:

  • made this PR as small as possible
  • unit-tested any new functionality
  • linked any relevant issues or PRs
  • [ ] included screenshots (if this involves a front-end change)

fix #1619

Summary by CodeRabbit

  • New Features
    • Introduced new WebAssembly bindings for cryptographic and order-related utilities, including functions for hashing and calldata encoding, with improved error handling.
  • Bug Fixes
    • Enhanced test cases for Rain Orderbook Bindings to provide clearer error and success condition checks.
  • Chores
    • Updated dependency for wasm-bindgen-utils to use a specific commit from a Git repository.

@findolor findolor added test test code rust Related to rust crates labels May 1, 2025
@findolor findolor added this to the Test coverage, tech debt milestone May 1, 2025
@findolor findolor requested a review from hardyjosh May 1, 2025 14:29
@findolor findolor self-assigned this May 1, 2025
Copy link
Contributor

coderabbitai bot commented May 1, 2025

Walkthrough

The changes migrate several WebAssembly bindings and utility functions related to cryptographic hashing and calldata encoding from the crates/bindings/src/js_api.rs file to a new module at crates/js_api/src/bindings/mod.rs. The new module provides improved error handling and result types for the exported functions, and introduces a custom error type for hex decoding failures. The Cargo.toml is updated to reference the wasm-bindgen-utils dependency via a specific Git commit. The test suite in the orderbook package is updated to use explicit checks for result values and errors when testing the bindings.

Changes

File(s) Change Summary
Cargo.toml Changed wasm-bindgen-utils dependency from version "0.0.7" to a Git reference with a pinned commit.
crates/bindings/src/js_api.rs Removed exported WASM functions for hashing and calldata utilities (get_order_hash, get_take_orders2_calldata, keccak256, keccak256_hex_string), their imports, and associated wasm-bindgen tests.
crates/js_api/src/bindings/mod.rs Added new module implementing WASM bindings for cryptographic and order utilities, with improved error handling, custom error type, and comprehensive wasm-bindgen tests. Exports include get_order_hash, get_take_orders2_calldata, keccak256, keccak256_hex_string, and a TakeOrdersCalldata wrapper.
crates/js_api/src/lib.rs Added conditional inclusion of the new bindings module for the WASM target family.
packages/orderbook/test/bindings/test.test.ts Updated tests to explicitly check for value and error properties in the results of getOrderHash, using chai's expect for clearer assertions and improved error handling.

Sequence Diagram(s)

sequenceDiagram
    participant JS as JavaScript
    participant WASM as WASM Bindings (bindings/mod.rs)
    participant Rust as Rust Core Logic

    JS->>WASM: getOrderHash(order)
    WASM->>Rust: ABI-encode order, compute keccak256 hash
    Rust-->>WASM: Hex string hash or error
    WASM-->>JS: Result (value or error)

    JS->>WASM: getTakeOrders2Calldata(config)
    WASM->>Rust: ABI-encode takeOrders2Call struct
    Rust-->>WASM: Calldata bytes or error
    WASM-->>JS: Result (calldata or error)

    JS->>WASM: keccak256(bytes)
    WASM->>Rust: Compute keccak256 hash
    Rust-->>WASM: Hex string hash or error
    WASM-->>JS: Result (value or error)

    JS->>WASM: keccak256HexString(hexString)
    WASM->>Rust: Decode hex, compute keccak256 hash
    Rust-->>WASM: Hex string hash or error
    WASM-->>JS: Result (value or error)
Loading

Assessment against linked issues

Objective Addressed Explanation
Add or improve missing tests for the Bindings crate, specifically js_api.rs (#1619)

Suggested reviewers

  • hardyjosh
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e02197d and c3937f0.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • tauri-app/src-tauri/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • Cargo.toml (1 hunks)
  • crates/bindings/src/js_api.rs (1 hunks)
  • crates/js_api/src/bindings/mod.rs (1 hunks)
  • crates/js_api/src/lib.rs (1 hunks)
  • packages/orderbook/test/bindings/test.test.ts (5 hunks)
🧰 Additional context used
🧠 Learnings (1)
crates/js_api/src/lib.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1715
File: crates/js_api/src/common/mod.rs:111-118
Timestamp: 2025-04-30T09:28:36.960Z
Learning: In the rain.orderbook repository, the WASM tests are already properly configured with conditional compilation using `#[cfg(target_family = "wasm")]` and `#[cfg(not(target_family = "wasm"))]`, and don't require additional `wasm_bindgen_test_configure!(run_in_browser)` directives.
⏰ Context from checks skipped due to timeout of 90000ms (16)
  • GitHub Check: test
  • GitHub Check: build-tauri (ubuntu-22.04, true)
  • GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
  • GitHub Check: Deploy-Preview
  • GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
  • GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
  • GitHub Check: test
  • GitHub Check: test
  • GitHub Check: git-clean
🔇 Additional comments (20)
crates/js_api/src/lib.rs (1)

1-2: New module correctly conditionally compiled for WebAssembly target

The bindings module is properly set up with conditional compilation using #[cfg(target_family = "wasm")] ensuring that WebAssembly-specific code is only included when targeting WASM. This aligns with the project's established pattern for WASM targeting as seen in other modules.

Cargo.toml (1)

56-56:

✅ Verification successful

Dependency updated to use Git reference

The wasm-bindgen-utils dependency is now referencing a specific Git commit rather than a fixed version. While this change is necessary to support the new WASM macro functionality, Git dependencies can introduce potential risks:

  1. The referenced repository could become unavailable
  2. The specific commit might be rebased or removed in the future

Consider eventually migrating to a released version once the required functionality is stabilized.


🌐 Web query:

What are the best practices for using Git dependencies versus published crates in Rust?

💡 Result:

When developing Rust projects, choosing between Git dependencies and published crates involves several considerations. Here's a comprehensive overview of best practices for both approaches.

Git Dependencies vs Published Crates

Git dependencies allow you to use code directly from repositories, while published crates come from centralized registries like crates.io. Each approach has specific use cases and considerations.

When to Use Git Dependencies

During Active Development
Git dependencies are particularly useful when you need to work with code that's actively being developed or when you need features not yet available in published versions[3]. They provide flexibility to test changes or bug fixes before they're officially released.

For Internal or Proprietary Libraries
When working with internal libraries that aren't meant for public consumption, Git dependencies provide a way to share code across projects without publishing to public registries[1][3].

Testing Unreleased Features
If you need access to bleeding-edge features or fixes from a dependency, you can point to specific branches, commits, or tags in a Git repository[4]:

[dependencies]
rand = { git = "https://github.com/rust-lang-nursery/rand", branch = "next" }

When to Use Published Crates

For Stable, Production Code
Published crates are generally more stable and reliable for production environments as they represent officially released versions that have typically undergone more testing[5].

For Public Projects
If you're planning to publish your crate to crates.io, all dependencies must also be from crates.io (except dev-dependencies)[1]. This is a requirement enforced by the registry.

For Better Reproducibility
Published crates with version numbers provide better reproducibility for builds, as the exact version is locked in your Cargo.lock file[5][6].

Best Practices

Use Multiple Locations for Development and Publishing

A powerful pattern in Rust is specifying both a local path/Git location and a version number[1]:

[dependencies]
# Uses local path when used locally, but version 1.0 from crates.io when published
bitflags = { path = "my-bitflags", version = "1.0" }

# Uses git repo when used locally, but version 1.0 from crates.io when published
smallvec = { git = "https://github.com/servo/rust-smallvec.git", version = "1.0" }

This approach allows you to:

  • Use local development versions during development
  • Automatically switch to published versions when your crate is published[1]
  • Test changes without needing to publish intermediate versions

Development Workflow Best Practices

For Active Development Across Multiple Projects
When developing multiple related projects where you need to modify shared code:

  1. Use path dependencies for local development[2]
  2. Switch to Git or published dependencies before committing code[2]
  3. Consider using workspace-relative paths when appropriate

Avoid Forgetting to Update Dependencies
A common pitfall is forgetting to update Git dependencies to published versions before committing[2]. Some developers create pre-commit hooks or review processes to catch these issues.

Publishing Considerations

Preparing for Publication
Before publishing to crates.io:

  • Ensure all dependencies are specified with version numbers
  • Remove any path-only dependencies (not allowed on crates.io)[1]
  • Document your public API carefully, as it becomes a commitment once published[9]
  • Use semantic versioning for your crate versions[6]

Version Control
Remember that published versions are permanent on crates.io and cannot be deleted or modified[9]. Plan your releases carefully.

Dependency Management for Organizations

For organizations requiring control over dependencies:

  • Consider setting up private registries for internal crates[7]
  • Use Git dependencies with specific commit hashes for better reproducibility
  • Explore tools like cargo vendor for offline builds or air-gapped environments[7][8]

By following these practices, you can effectively manage dependencies in your Rust projects while maintaining flexibility for development and stability for releases.

Citations:


Git dependency pinned for wasm-bindgen-utils—consider migrating to a published crate when stable

This change pins wasm-bindgen-utils to commit 4379d24a671742a58ec5cdab2c39fecbda59619d in Cargo.toml to enable the new WASM macro. While Git dependencies are a valid approach during active development or for unreleased/internal code, they carry some risks:

  • The upstream repository may become unavailable or be archived.
  • A pinned commit could be rebased, force-pushed, or removed in the future.

For long-term stability, reproducibility, and compliance with crates.io publishing requirements, plan to switch back to a released version once the functionality you need is stabilized. For example:

# After upstream release, update to a crates.io version
wasm-bindgen-utils = "x.y.z"
packages/orderbook/test/bindings/test.test.ts (2)

4-4: Appropriate inclusion of Chai expectations library

Adding the expect functionality from Chai provides improved assertion capabilities for testing the new result structure returned by the WASM bindings.


32-32: Result handling correctly updated for success case

The test properly checks for the presence of a value property in the result, aligning with the new result-based error handling pattern implemented in the WASM bindings.

Also applies to: 34-34

crates/bindings/src/js_api.rs (2)

2-2: Removed unused import types

The removed types from line 2 is consistent with the broader change of relocating the WebAssembly bindings from this file to the JS API crate, as described in the PR objectives.


1-182:

✅ Verification successful

Verify that all removed functionalities are properly reimplemented

According to the AI summary, several WebAssembly utility functions (get_order_hash, get_take_orders2_calldata, keccak256, and keccak256_hex_string) were removed from this file and relocated to crates/js_api/src/bindings/mod.rs. We should verify that all functionality has been properly implemented in the new location with appropriate error handling.


🏁 Script executed:

#!/bin/bash
# Description: Verify that all removed functions have been implemented in the new file location

# Check for the new implementation file
if [ ! -f "crates/js_api/src/bindings/mod.rs" ]; then
  echo "ERROR: New implementation file not found at crates/js_api/src/bindings/mod.rs"
  exit 1
fi

# Check for removed function implementations in the new file
echo "Checking for implementation of removed functions in new location:"
echo "---"

# Check for get_order_hash function
echo "get_order_hash:"
grep -A 10 "get_order_hash" crates/js_api/src/bindings/mod.rs || echo "Not found!"

# Check for get_take_orders2_calldata function
echo "---"
echo "get_take_orders2_calldata:"
grep -A 10 "get_take_orders2_calldata" crates/js_api/src/bindings/mod.rs || echo "Not found!"

# Check for keccak256 function
echo "---"
echo "keccak256:"
grep -A 10 "keccak256(" crates/js_api/src/bindings/mod.rs || echo "Not found!"

# Check for keccak256_hex_string function
echo "---"
echo "keccak256_hex_string:"
grep -A 10 "keccak256_hex_string" crates/js_api/src/bindings/mod.rs || echo "Not found!"

# Check for error types which would indicate improved error handling
echo "---"
echo "Error handling types:"
grep -A 10 "Error" crates/js_api/src/bindings/mod.rs || echo "No explicit error handling found!"

Length of output: 6618


Removed Utility Functions Successfully Migrated

All four WebAssembly helpers—get_order_hash, get_take_orders2_calldata (exported as getTakeOrders2Calldata), keccak256, and keccak256_hex_string (exported as keccak256HexString)—are present in crates/js_api/src/bindings/mod.rs with the correct signatures returning Result<…, Error>.
The custom Error enum covers hex‐decode failures, provides a to_readable_msg method, and is converted into both JsValue and WasmEncodedError. Comprehensive tests verify success and failure paths.

No further changes needed.

crates/js_api/src/bindings/mod.rs (14)

1-14: Well-organized imports with appropriate dependencies.

The imports are correctly organized and include all necessary dependencies for cryptographic operations, type conversions, and WebAssembly bindings.


15-17: Good struct definition with appropriate traits.

The TakeOrdersCalldata struct is properly defined with serialization traits and TypeScript type annotations. The use of impl_wasm_traits! macro ensures all necessary WASM-related traits are implemented.


19-23: Clear and well-implemented order hash function.

The get_order_hash function correctly encodes and hashes the order data. The use of wasm_export macro with appropriate JS name and return type is a good implementation of the PR objectives.


25-38: Well-structured function with good parameter handling.

The get_take_orders2_calldata function correctly creates and encodes the calldata. The return type wrapping with TakeOrdersCalldata provides a clean interface for JavaScript consumers.


40-44: Simple and effective keccak256 implementation.

The function provides a straightforward way to hash raw bytes and return a hex-encoded result, which is useful for JavaScript interoperability.


46-50: Good error propagation in hex string handling.

The function correctly uses the ? operator to propagate potential hex decoding errors, ensuring proper error handling for invalid inputs.


52-56: Well-defined error type using thiserror.

The error enum is properly defined with thiserror for automatic trait implementations. The error handling is focused on hex decoding errors, which is appropriate for the current functionality.


58-64: Good error message formatting.

The to_readable_msg method provides user-friendly error messages, which improves the developer experience when using these bindings.


66-70: Proper JsValue error conversion.

The implementation correctly converts the custom error type to a JavaScript error value, which is essential for proper error handling in JavaScript.


72-79: Comprehensive WasmEncodedError conversion.

The implementation provides both a technical and a readable error message, which enhances debugging capabilities for JavaScript consumers.


81-94: Good test coverage for order hash function.

The test verifies the expected hash output against a known value, ensuring the function works correctly.


96-105: Thorough test for calldata generation.

The test correctly verifies that the generated calldata matches the expected ABI-encoded output, ensuring compatibility with the smart contract interface.


107-114: Effective test for basic keccak256 function.

The test verifies the hashing function against a known expected output for a simple input.


116-130: Comprehensive error handling test.

The test covers both the success case and error case for the hex string hashing function, ensuring proper error messages are generated for invalid inputs.

Comment on lines +63 to 64
if (!result.error) expect.fail('expected to error, but did not');
assert.fail('expected to error, but resolved');
Copy link
Contributor

@coderabbitai coderabbitai bot May 1, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Improved error handling pattern, but contains redundant code

The test now correctly checks for the presence of an error property in the result, but the subsequent assert.fail() line will never be reached if the condition passes. This creates redundant code that could be misleading.

Consider simplifying the error tests by removing the unreachable assert.fail() calls:

-			if (!result.error) expect.fail('expected to error, but did not');
-			assert.fail('expected to error, but resolved');
+			if (!result.error) expect.fail('expected to error, but did not');

Also applies to: 97-98, 133-134

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can keep this it's okay

Copy link
Contributor

Choose a reason for hiding this comment

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

@findolor Understood! No problem keeping the redundant assertions as they are. They don't harm functionality and can serve as additional documentation of intent. Thanks for considering the suggestion.

Comment on lines +1 to +131
) -> Result<TakeOrdersCalldata, Error> {
let calldata = takeOrders2Call {
config: take_orders_config,
}
.abi_encode();
Ok(TakeOrdersCalldata(Bytes::copy_from_slice(&calldata)))
}

/// calculates keccak256 of the given bytes
#[wasm_export(js_name = "keccak256", unchecked_return_type = "string")]
pub fn keccak256(bytes: &[u8]) -> Result<String, Error> {
Ok(encode_prefixed(main_keccak256(bytes)))
}

/// calculate keccak256 of a hex string
#[wasm_export(js_name = "keccak256HexString", unchecked_return_type = "string")]
pub fn keccak256_hex_string(hex_string: &str) -> Result<String, Error> {
Ok(encode_prefixed(main_keccak256(decode(hex_string)?)))
}

#[derive(Debug, thiserror::Error)]
pub enum Error {
#[error("Failed to decode hex string")]
FromHexError(#[from] FromHexError),
}

impl Error {
pub fn to_readable_msg(&self) -> String {
match self {
Self::FromHexError(e) => format!("Failed to decode hex string: {}", e),
}
}
}

impl From<Error> for JsValue {
fn from(value: Error) -> Self {
JsError::new(&value.to_string()).into()
}
}

impl From<Error> for WasmEncodedError {
fn from(value: Error) -> Self {
WasmEncodedError {
msg: value.to_string(),
readable_msg: value.to_readable_msg(),
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use wasm_bindgen_test::wasm_bindgen_test;

#[wasm_bindgen_test]
fn test_get_order_hash() {
let order = OrderV3::default();
let result = get_order_hash(&order).unwrap();
assert_eq!(
result,
"0xdcf6b886b1922d32accc60b1a0cdc53fb4bcbe74af2987b22046820030e3423b"
);
}

#[wasm_bindgen_test]
fn test_take_orders_calldata() {
let take_orders_config = TakeOrdersConfigV3::default();
let result = get_take_orders2_calldata(take_orders_config.clone()).unwrap();
let expected = takeOrders2Call {
config: take_orders_config,
}
.abi_encode();
assert_eq!(result.0.to_vec(), expected);
}

#[wasm_bindgen_test]
fn test_keccak256() {
let bytes = vec![1, 2];
let result = keccak256(&bytes).unwrap();
let expected =
"0x22ae6da6b482f9b1b19b0b897c3fd43884180a1c5ee361e1107a1bc635649dda".to_string();
assert_eq!(result, expected);
}

#[wasm_bindgen_test]
fn test_keccak256_hex_string() {
let hex_string = "0x0102";
let result = keccak256_hex_string(&hex_string).unwrap();
let expected =
"0x22ae6da6b482f9b1b19b0b897c3fd43884180a1c5ee361e1107a1bc635649dda".to_string();
assert_eq!(result, expected);

let err = keccak256_hex_string("invalid-hex").unwrap_err();
assert_eq!(err.to_string(), "Failed to decode hex string");
assert_eq!(
err.to_readable_msg(),
"Failed to decode hex string: Odd number of digits"
);
}
}
Copy link
Contributor

@coderabbitai coderabbitai bot May 1, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider adding input validation for empty inputs.

While the implementation is solid, it might be beneficial to add explicit validation for empty inputs in functions like keccak256 and get_order_hash to provide more specific error messages.

For example, you could add validation in the keccak256 function:

pub fn keccak256(bytes: &[u8]) -> Result<String, Error> {
+    if bytes.is_empty() {
+        return Err(Error::EmptyInput("Cannot hash empty byte array".to_string()));
+    }
    Ok(encode_prefixed(main_keccak256(bytes)))
}

And update the Error enum:

pub enum Error {
    #[error("Failed to decode hex string")]
    FromHexError(#[from] FromHexError),
+    #[error("{0}")]
+    EmptyInput(String),
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
use alloy::hex::FromHexError;
use alloy::primitives::bytes::Bytes;
use alloy::sol_types::SolValue;
use alloy::{
primitives::{
hex::{decode, encode_prefixed},
keccak256 as main_keccak256,
},
sol_types::SolCall,
};
use rain_orderbook_bindings::IOrderBookV4::{takeOrders2Call, OrderV3, TakeOrdersConfigV3};
use serde::{Deserialize, Serialize};
use wasm_bindgen_utils::{impl_wasm_traits, prelude::*};
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Tsify)]
pub struct TakeOrdersCalldata(#[tsify(type = "string")] Bytes);
impl_wasm_traits!(TakeOrdersCalldata);
/// Get the order hash of an order
#[wasm_export(js_name = "getOrderHash", unchecked_return_type = "string")]
pub fn get_order_hash(order: &OrderV3) -> Result<String, Error> {
Ok(encode_prefixed(main_keccak256(order.abi_encode())))
}
/// Get takeOrders2() calldata
#[wasm_export(
js_name = "getTakeOrders2Calldata",
unchecked_return_type = "TakeOrdersCalldata"
)]
pub fn get_take_orders2_calldata(
take_orders_config: TakeOrdersConfigV3,
) -> Result<TakeOrdersCalldata, Error> {
let calldata = takeOrders2Call {
config: take_orders_config,
}
.abi_encode();
Ok(TakeOrdersCalldata(Bytes::copy_from_slice(&calldata)))
}
/// calculates keccak256 of the given bytes
#[wasm_export(js_name = "keccak256", unchecked_return_type = "string")]
pub fn keccak256(bytes: &[u8]) -> Result<String, Error> {
Ok(encode_prefixed(main_keccak256(bytes)))
}
/// calculate keccak256 of a hex string
#[wasm_export(js_name = "keccak256HexString", unchecked_return_type = "string")]
pub fn keccak256_hex_string(hex_string: &str) -> Result<String, Error> {
Ok(encode_prefixed(main_keccak256(decode(hex_string)?)))
}
#[derive(Debug, thiserror::Error)]
pub enum Error {
#[error("Failed to decode hex string")]
FromHexError(#[from] FromHexError),
}
impl Error {
pub fn to_readable_msg(&self) -> String {
match self {
Self::FromHexError(e) => format!("Failed to decode hex string: {}", e),
}
}
}
impl From<Error> for JsValue {
fn from(value: Error) -> Self {
JsError::new(&value.to_string()).into()
}
}
impl From<Error> for WasmEncodedError {
fn from(value: Error) -> Self {
WasmEncodedError {
msg: value.to_string(),
readable_msg: value.to_readable_msg(),
}
}
}
#[cfg(test)]
mod tests {
use super::*;
use wasm_bindgen_test::wasm_bindgen_test;
#[wasm_bindgen_test]
fn test_get_order_hash() {
let order = OrderV3::default();
let result = get_order_hash(&order).unwrap();
assert_eq!(
result,
"0xdcf6b886b1922d32accc60b1a0cdc53fb4bcbe74af2987b22046820030e3423b"
);
}
#[wasm_bindgen_test]
fn test_take_orders_calldata() {
let take_orders_config = TakeOrdersConfigV3::default();
let result = get_take_orders2_calldata(take_orders_config.clone()).unwrap();
let expected = takeOrders2Call {
config: take_orders_config,
}
.abi_encode();
assert_eq!(result.0.to_vec(), expected);
}
#[wasm_bindgen_test]
fn test_keccak256() {
let bytes = vec![1, 2];
let result = keccak256(&bytes).unwrap();
let expected =
"0x22ae6da6b482f9b1b19b0b897c3fd43884180a1c5ee361e1107a1bc635649dda".to_string();
assert_eq!(result, expected);
}
#[wasm_bindgen_test]
fn test_keccak256_hex_string() {
let hex_string = "0x0102";
let result = keccak256_hex_string(&hex_string).unwrap();
let expected =
"0x22ae6da6b482f9b1b19b0b897c3fd43884180a1c5ee361e1107a1bc635649dda".to_string();
assert_eq!(result, expected);
let err = keccak256_hex_string("invalid-hex").unwrap_err();
assert_eq!(err.to_string(), "Failed to decode hex string");
assert_eq!(
err.to_readable_msg(),
"Failed to decode hex string: Odd number of digits"
);
}
}
--- a/crates/js_api/src/bindings/mod.rs
+++ b/crates/js_api/src/bindings/mod.rs
@@
/// calculates keccak256 of the given bytes
#[wasm_export(js_name = "keccak256", unchecked_return_type = "string")]
pub fn keccak256(bytes: &[u8]) -> Result<String, Error> {
+ if bytes.is_empty() {
+ return Err(Error::EmptyInput("Cannot hash empty byte array".to_string()));
+ }
Ok(encode_prefixed(main_keccak256(bytes)))
}
@@
#[derive(Debug, thiserror::Error)]
pub enum Error {
#[error("Failed to decode hex string")]
FromHexError(#[from] FromHexError),
+ #[error("{0}")]
+ EmptyInput(String),
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't need to do this

Copy link
Contributor

Choose a reason for hiding this comment

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

@findolor Understood! I'll close this suggestion. Thank you for the clarification.


✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1721
File: crates/js_api/src/bindings/mod.rs:1-131
Timestamp: 2025-05-01T14:39:53.772Z
Learning: Empty input validation for functions like `keccak256` and `get_order_hash` in the WebAssembly bindings is not required as empty inputs are considered valid in this context.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

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

Successfully merging this pull request may close these issues.

Missing Test - Bindings crate - js_api.rs
1 participant