refactor: rename host functions#7338
Open
mvadari wants to merge 4 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns rippled’s WASM host-function import names and related WASM fixtures/tests with the renames described in the referenced XRPL-Standards update, ensuring the engine and fixture contracts agree on the new symbols.
Changes:
- Renamed many host function import strings in the WASM VM and updated C++ unit tests accordingly.
- Updated Rust/C WASM fixture sources (and regenerated fixture/lock outputs) to import/call the renamed host functions.
- Enhanced the fixture-copying script to better locate/update fixture constants and added WAT fixture support.
Reviewed changes
Copilot reviewed 18 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/app/Wasm_test.cpp | Updates expected import names, section offsets, and instruction allowances for updated fixtures. |
| src/test/app/wasm_fixtures/ledgerSqn.c | Renames imported host function to ldgr_index. |
| src/test/app/wasm_fixtures/float_tests/src/lib.rs | Updates to renamed float/ledger-entry host APIs and reformats some code. |
| src/test/app/wasm_fixtures/float_tests/Cargo.toml | Switches xrpl-std dependency to the renames branch. |
| src/test/app/wasm_fixtures/float_tests/Cargo.lock | Updates resolved deps for the new xrpl-std revision and crate renames. |
| src/test/app/wasm_fixtures/float_0/src/lib.rs | Updates float host function names (float_cmp, float_sub). |
| src/test/app/wasm_fixtures/float_0/Cargo.toml | Changes xrpl-std dependency spec (now branch-based). |
| src/test/app/wasm_fixtures/float_0/Cargo.lock | Updates resolved deps for the new xrpl-std revision and crate renames. |
| src/test/app/wasm_fixtures/copyFixtures.py | Improves fixture name discovery and adds WAT/WAT-zip build support. |
| src/test/app/wasm_fixtures/codecov_tests/src/lib.rs | Renames all exercised host calls to match new host function names. |
| src/test/app/wasm_fixtures/codecov_tests/src/host_bindings_loose.rs | Updates “loose” bindings to renamed imports. |
| src/test/app/wasm_fixtures/codecov_tests/Cargo.toml | Switches xrpl-std dependency to the renames branch. |
| src/test/app/wasm_fixtures/codecov_tests/Cargo.lock | Updates resolved deps for the new xrpl-std revision and crate renames. |
| src/test/app/wasm_fixtures/bad_align.c | Updates check_keylet import to check_id. |
| src/test/app/wasm_fixtures/all_keylets/src/lib.rs | Updates keylet helpers and trace host function names for renamed APIs. |
| src/test/app/wasm_fixtures/all_keylets/Cargo.toml | Switches xrpl-std dependency to the renames branch. |
| src/test/app/wasm_fixtures/all_keylets/Cargo.lock | Updates resolved deps for the new xrpl-std revision and crate renames. |
| src/test/app/wasm_fixtures/all_host_functions/src/lib.rs | Updates calls to renamed host APIs (some strings/comments still reference old names). |
| src/test/app/wasm_fixtures/all_host_functions/Cargo.toml | Switches xrpl-std dependency to the renames branch. |
| src/test/app/wasm_fixtures/all_host_functions/Cargo.lock | Updates resolved deps for the new xrpl-std revision and crate renames. |
| src/test/app/HostFuncImpl_test.cpp | Updates import lookups for renamed host functions (one critical mismatch found). |
| src/libxrpl/tx/wasm/WasmVM.cpp | Renames imported host symbols exposed to WASM (core of the refactor). |
| cspell.config.yaml | Adds xfloat to dictionary for renamed trace function. |
Comments suppressed due to low confidence (1)
src/test/app/wasm_fixtures/all_host_functions/src/lib.rs:781
- The failure message still says
update_data failedeven though the call is toset_data. Update the error string so logs match the renamed host function.
if update_result != update_payload.len() as i32 {
let _ = trace_num("ERROR: update_data failed:", update_result as i64);
return -701; // Data update failed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto* trap = | ||
| ww(floatFromMantExp_wrap, | ||
| &import.at("float_compare"), | ||
| &import.at("float_cmp"), |
|
|
||
| [dependencies] | ||
| xrpl-std = { git = "https://github.com/ripple/xrpl-wasm-stdlib.git", package = "xrpl-wasm-stdlib", rev = "1e5d096f46742ef7fcf1cb6f28a2526a72ed59d8" } | ||
| xrpl-std = { git = "https://github.com/ripple/xrpl-wasm-stdlib.git", package = "xrpl-wasm-stdlib", branch = "renames" } |
Comment on lines
145
to
+149
| // Test 2.1: get_tx_field() - Basic transaction field access | ||
| // Test with Account field (required, 20 bytes) | ||
| let mut account_buffer = [0u8; 20]; | ||
| let account_len = unsafe { | ||
| host::get_tx_field( | ||
| sfield::Account, | ||
| host::tx_field( |
Comment on lines
700
to
704
| // Test 6.2: get_nft() - NFT data retrieval | ||
| let escrow_finish = EscrowFinish; | ||
| let account_id = escrow_finish.get_account().unwrap(); | ||
| let nft_id = [0u8; 32]; // Dummy NFT ID for testing | ||
| let mut nft_buffer = [0u8; 256]; |
Comment on lines
774
to
778
| // Test 7.1: update_data() - Update current ledger entry data | ||
| let update_payload = b"Updated ledger entry data from WASM test"; | ||
|
|
||
| let update_result = unsafe { host::update_data(update_payload.as_ptr(), update_payload.len()) }; | ||
| let update_result = unsafe { host::set_data(update_payload.as_ptr(), update_payload.len()) }; | ||
|
|
Comment on lines
682
to
686
| let test_data = b"Hello, XRPL WASM world!"; | ||
| let mut hash_output = [0u8; 32]; | ||
| let hash_result = unsafe { | ||
| host::compute_sha512_half( | ||
| host::sha512_half( | ||
| test_data.as_ptr(), |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ripple/wasmi-host-functions #7338 +/- ##
=============================================================
- Coverage 82.5% 82.5% -0.0%
=============================================================
Files 1024 1024
Lines 78291 78291
Branches 7437 7443 +6
=============================================================
- Hits 64575 64565 -10
- Misses 13716 13726 +10
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
| wat_files = [f for f in os.listdir(base_path) if f.endswith(".wat")] | ||
| wat_path = os.path.join(base_path, "wat") | ||
| wat_fixture_files = [ | ||
| f for f in os.listdir(wat_path) if f.endswith((".wat", ".zip")) |
Contributor
There was a problem hiding this comment.
os.listdir(wat_path) crashes with FileNotFoundError if wat/ doesn't exist. Add a guard:
wat_path = os.path.join(base_path, "wat")
wat_fixture_files = (
[f for f in os.listdir(wat_path) if f.endswith((".wat", ".zip"))]
if os.path.isdir(wat_path)
else []
)
| "4f41545f5a45524f20636f6d706172653a206261642020666c6f61742031302d31303a206661696c6564004d097072" | ||
| "6f64756365727302086c616e6775616765010452757374000c70726f6365737365642d6279010572757374631d312e" | ||
| "38372e30202831373036376539616320323032352d30352d303929002c0f7461726765745f6665617475726573022b" | ||
| "0f6d757461626c652d676c6f62616c732b087369676e2d657874"; | ||
|
|
||
| extern std::string const kDisabledFloatHex = | ||
| "0061736d010000000108026000006000017f03030200010503010002063e0a7f004180080b7f004180080b7f004180" |
Contributor
There was a problem hiding this comment.
Duplicate variable definitions will cause compilation errors. kFibWasmHex, kLedgerSqnWasmHex, kAllHostFunctionsWasmHex, and others are already defined earlier in this file with updated hex content and are redefined again here. Remove these duplicate definitions — only the new variables (kFloat0WasmHex, kThousandParamsWasmHex, kThousand1ParamsWasmHex, kInfiniteLoopWasmHex) should be added.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Title says it all
Context of Change
Changes made according to this PR: XRPLF/XRPL-Standards#531
API Impact
N/A