-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: add WASMI dependency #5999
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
base: develop
Are you sure you want to change the base?
Conversation
|
@mvadari we would normally upload our custom recipes to https://github.com/XRPLF/conan-center-index |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5999 +/- ##
=========================================
- Coverage 79.1% 79.1% -0.0%
=========================================
Files 836 836
Lines 71261 71261
Branches 8301 8303 +2
=========================================
- Hits 56399 56393 -6
- Misses 14862 14868 +6 🚀 New features to boost your workflow:
|
|
|
||
| - name: Prepare runner | ||
| uses: XRPLF/actions/.github/actions/prepare-runner@99685816bb60a95a66852f212f382580e180df3a | ||
| uses: XRPLF/actions/.github/actions/prepare-runner@ff9f8f649df5855ffe1a1ae715df43e51807f2e0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change here and in the other CI file still required? I suspect this is an older hash than what is currently used by the develop branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds WASMI (WebAssembly interpreter) version 0.42.1 as a dependency to rippled in preparation for the Smart Escrows feature (XLS-102). The integration includes custom patches to the upstream WASMI library, Conan package configuration, and updates to the build system and CI/CD pipelines.
Key Changes
- Added WASMI 0.42.1 dependency with custom patches for C API modifications and build system integration
- Integrated WASMI into the Conan build system with local recipe export workflow
- Updated CI/CD Docker images (SHA:
ca4517d) and build scripts to support WASMI compilation
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| external/wasmi/patches/0001-xrplf-0.42.1.patch | Custom patch for WASMI 0.42.1 adding fuel management APIs and build system modifications for static linking |
| external/wasmi/conanfile.py | Conan recipe for building WASMI from source with CMake integration |
| external/wasmi/conandata.yml | Conan data file specifying WASMI source URL, SHA256, and patch metadata |
| conanfile.py | Added wasmi/0.42.1 to project dependencies and package info |
| conan.lock | Removed lockfile (will be regenerated during build) |
| cmake/XrplCore.cmake | Added wasmi::wasmi to xrpl.imports.main link libraries |
| CMakeLists.txt | Added find_package(wasmi REQUIRED) to locate WASMI during configuration |
| BUILD.md | Added instructions for exporting WASMI Conan recipe locally |
| .github/workflows/upload-conan-deps.yml | Updated prepare-runner action reference to newer SHA |
| .github/workflows/reusable-build-test-config.yml | Updated prepare-runner action reference to newer SHA |
| .github/scripts/strategy-matrix/linux.json | Updated all Docker image SHAs to ca4517d for Linux build matrix |
| .github/actions/build-deps/action.yml | Added local wasmi export step before conan install |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| + store: &mut wasm_store_t, | ||
| + fuel: u64, | ||
| +) -> Option<Box<wasmi_error_t>> { | ||
| + |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace on empty line. This should be removed to maintain code cleanliness.
| +//WASM_API_EXTERN void *wasm_store_get_data(const wasm_store_t*); | ||
| +//WASM_API_EXTERN void wasm_store_set_data(wasm_store_t*, void *data); |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented-out code should be removed rather than left in the codebase. If these functions are needed in the future, they can be retrieved from version control. If they're not intended to be used yet, consider removing them from the patch entirely.
| +//WASM_API_EXTERN void *wasm_store_get_data(const wasm_store_t*); | |
| +//WASM_API_EXTERN void wasm_store_set_data(wasm_store_t*, void *data); |
| +//////////////////////////////////////////////////////////////////////////////////////// | ||
| +//////////////////////////////////////////////////////////////////////////////////////// | ||
| +//////////////////////////////////////////////////////////////////////////////////////// |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Excessive visual separator comment lines (4 lines of forward slashes). This adds clutter without providing value. Consider reducing to a single line separator or using a more conventional Rust doc comment style.
| +//////////////////////////////////////////////////////////////////////////////////////// | |
| +//////////////////////////////////////////////////////////////////////////////////////// | |
| +//////////////////////////////////////////////////////////////////////////////////////// |
| LOG_VERBOSITY: ${{ inputs.log_verbosity }} | ||
| run: | | ||
| echo 'Installing dependencies.' | ||
| conan export --version 0.42.1 external/wasmi #TODO Switch to conan package |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The TODO comment suggests this is a temporary workaround. Consider creating a GitHub issue to track the work needed to switch to a conan package instead of relying on local export, and reference it in the comment (e.g., #TODO: Switch to conan package (issue #XXXX)).
| conan export --version 0.42.1 external/wasmi #TODO Switch to conan package | |
| conan export --version 0.42.1 external/wasmi #TODO: Switch to conan package (issue #1234) |
external/wasmi/conanfile.py
Outdated
| @@ -0,0 +1,48 @@ | |||
| from conan import ConanFile, tools | |||
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import: tools is imported from conan but never used in this file. This should be removed to keep the imports clean.
| from conan import ConanFile, tools | |
| from conan import ConanFile |
external/wasmi/conanfile.py
Outdated
| self.cpp_info.names["cmake_find_package"] = "wasmi" | ||
| self.cpp_info.names["cmake_find_package_multi"] = "wasmi" |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cpp_info.names attributes (cmake_find_package and cmake_find_package_multi) are deprecated in Conan 2.0 and should not be used. These attributes were for Conan 1.x compatibility. Since this recipe specifies required_conan_version = ">=2.0.0", these lines should be removed. Conan 2.0 uses the package name directly.
| self.cpp_info.names["cmake_find_package"] = "wasmi" | |
| self.cpp_info.names["cmake_find_package_multi"] = "wasmi" |
BUILD.md
Outdated
| conan export --version 0.42.1 external/wasmi | ||
| rm -rf .git | ||
| ``` | ||
|
|
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect path for conan export. The instruction should be conan export --version 0.42.1 external/wasmi but it's located in a section where other exports use paths like recipes/ed25519/all. This instruction appears in the wrong location - it's shown after checking out recipes from the conan-center-index repository, but external/wasmi is part of the rippled repository itself, not the conan-center-index. This line should be moved outside the git sparse-checkout section, or the section should be clarified to indicate when to run this command.
| conan export --version 0.42.1 external/wasmi | |
| rm -rf .git | |
| ``` | |
| rm -rf .git |
Exporting the local wasmi recipe
The wasmi recipe is part of the rippled repository itself, not the conan-center-index. To export it, run:
conan export --version 0.42.1 external/wasmi* Use conan repo for wasmi lib * Generate lockfile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Could you please resolve the conflict? |
@vvysokikh1 done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bthomee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I'll hold off with approving until the Conan recipe has been updated to use the newly created wasmi fork.
High Level Overview of Change
This PR adds Wasmi as a dependency to rippled, in preparation for Smart Escrows.
Context of Change
Splitting up #5600 into more managably reviewable chunks
XLS-102
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
N/A
Test Plan
CI passes and everything compiles in properly without error.
Future Tasks
Additional PRs for the rest of the Smart Escrow functionality