fix(wasm-sdk): resolve CI test failures and build issues #2765
Conversation
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
Broken by a recent commit
Started failing once an implementation was available since the test expected it to be unimplemented.
Previously tests always indicated successful even if a test failed.
The redundant actions/cache/save@v4 steps for protoc and wasm-opt were causing "Path Validation Error" warnings when cache hits occurred. The actions/cache@v4 action automatically handles both restore and save operations, making the separate save steps unnecessary. Issue was introduced in #2756
WalkthroughRemoved two cache-save steps from the wasm-sdk CI workflow, added pipefail to WASM SDK test runs, updated wasm-sdk tests to implement child-key derivation and xprv→xpub behavior checks, and changed the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor GH as GitHub Actions
participant CI as workflow job
participant Cache as Cache
participant DL as Binaryen Release
participant Runner as Job Runner
Note over GH,CI: Version-driven Binaryen/wasm-opt install & cache
GH->>CI: start wasm-sdk build
CI->>Cache: restore cache `binaryen/${BINARYEN_VERSION}/${{runner.os}}/${BINARYEN_ARCH}`
alt cache miss
CI->>DL: download binaryen-${BINARYEN_VERSION}-${BINARYEN_ARCH}-linux.tar.gz
DL-->>CI: tarball
CI->>CI: verify checksum (BINARYEN_SHA256)
CI->>Runner: extract & rename to `${BINARYEN_VERSION}`
end
CI->>Runner: export PATH `${HOME}/.cache/binaryen/${BINARYEN_VERSION}/bin`
CI->>Runner: run build & tests (with `set -o pipefail`)
sequenceDiagram
autonumber
actor TestRunner as Test Runner
participant WasmSDK as Wasm SDK
participant Node as Platform Node
Note over TestRunner,WasmSDK: Updated get_contested_resources call (8 args)
TestRunner->>WasmSDK: get_contested_resources(docType, contractId, indexName, startAt, limit, offset, orderAsc)
WasmSDK->>Node: query contested resources
Node-->>WasmSDK: results
WasmSDK-->>TestRunner: contested resources list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-09-07T22:18:50.883ZApplied to files:
📚 Learning: 2025-07-31T18:15:51.813ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
✨ Finishing touches🧪 Generate unit tests
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. Comment |
Reverts the downgrade to version 121 that was introduced in #2756. The newer version 124 should have better optimization algorithms and support more flags, reducing compatibility testing overhead and improving build performance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
thephez
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/wasm-sdk/test/key-generation.test.mjs (4)
364-379: Strengthen assertions and add hardened-derivation negative case.Good functional coverage. Two small enhancements will harden the test:
- Assert the serialized format prefix (xpub) to catch version/serialization regressions.
- Add a test that hardened derivation from an xpub is rejected.
Apply this small inline assertion:
if (!childXpub) throw new Error('No child xpub returned'); if (typeof childXpub !== 'string') throw new Error('Child xpub should be string'); + if (!childXpub.startsWith('xpub')) throw new Error(`Expected xpub prefix, got ${childXpub.slice(0,4)}`); if (childXpub === parentXpub) throw new Error('Child xpub should differ from parent');Add this new negative test (outside this hunk, e.g., right after this test):
await test('derive_child_public_key - hardened index should error from xpub', () => { const master = wasmSdk.derive_key_from_seed_with_extended_path(TEST_MNEMONIC, null, "m/44'/5'/0'", "mainnet"); try { wasmSdk.derive_child_public_key(master.xpub, 0, true); throw new Error('Should have thrown for hardened child from xpub'); } catch (error) { const msg = String(error?.message || error); if (!/harden(ed)?|public.*cannot.*derive/i.test(msg)) throw error; } });Note: Using the synchronous wrapper aligns with prior guidance; no await needed here.
381-394: Also assert xpub prefix for the round-trip.Validates serialization/version bytes, not just equality.
if (!derivedXpub) throw new Error('No xpub returned'); if (typeof derivedXpub !== 'string') throw new Error('Derived xpub should be string'); + if (!derivedXpub.startsWith('xpub')) throw new Error(`Expected xpub prefix, got ${derivedXpub.slice(0,4)}`); if (derivedXpub !== expectedXpub) throw new Error('Derived xpub should match expected xpub');
395-404: Avoid brittle coupling to exact error strings.Error messages can vary across versions/bindings. Prefer a tolerant regex or an error code if exposed.
- } catch (error) { - if (!error.message.includes('Invalid extended public key')) { - throw error; - } - } + } catch (error) { + const msg = String(error?.message || error); + if (!/invalid.*extended.*public/i.test(msg)) { + throw error; + } + }
406-415: Same here: relax the error assertion for xprv_to_xpub.Prevents false negatives on minor wording shifts.
- } catch (error) { - if (!error.message.includes('Invalid extended private key')) { - throw error; - } - } + } catch (error) { + const msg = String(error?.message || error); + if (!/invalid.*extended.*private/i.test(msg)) { + throw error; + } + }.github/workflows/wasm-sdk-build.yml (1)
106-121: Binaryen v124 bump looks consistent; consider parameterizing version/arch and adding checksum verification.
- Consistent path/key/PATH updates to version_124 look correct.
- Optional: reduce future churn by centralizing version/arch; add SHA256 verification for supply-chain hygiene.
Example minimal tweaks:
jobs: build-wasm-sdk: runs-on: ubuntu-latest + env: + BINARYEN_VERSION: version_124 + BINARYEN_ARCH: x86_64 # or use ${{ runner.arch == 'ARM64' && 'arm64' || 'x86_64' }} @@ - with: - path: ${{ env.HOME }}/.cache/binaryen/version_124 - key: binaryen/version_124/${{ runner.os }}/x86_64 + with: + path: ${{ env.HOME }}/.cache/binaryen/${{ env.BINARYEN_VERSION }} + key: binaryen/${{ env.BINARYEN_VERSION }}/${{ runner.os }}/${{ env.BINARYEN_ARCH }} @@ - curl -fsSL -o /tmp/binaryen.tar.gz \ - "https://github.com/WebAssembly/binaryen/releases/download/version_124/binaryen-version_124-x86_64-linux.tar.gz" + curl -fsSL -o /tmp/binaryen.tar.gz \ + "https://github.com/WebAssembly/binaryen/releases/download/${BINARYEN_VERSION}/binaryen-${BINARYEN_VERSION}-${BINARYEN_ARCH}-linux.tar.gz" + # Optional: verify checksum (replace EXPECTED_SHA256 with release value) + echo "EXPECTED_SHA256 /tmp/binaryen.tar.gz" | sha256sum -c - @@ - mv "${HOME}/.cache/binaryen/binaryen-version_124" "${HOME}/.cache/binaryen/version_124" + mv "${HOME}/.cache/binaryen/binaryen-${BINARYEN_VERSION}" "${HOME}/.cache/binaryen/${BINARYEN_VERSION}" @@ - echo "${HOME}/.cache/binaryen/version_124/bin" >> $GITHUB_PATH + echo "${HOME}/.cache/binaryen/${BINARYEN_VERSION}/bin" >> $GITHUB_PATH
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/wasm-sdk-build.yml(2 hunks)packages/wasm-sdk/test/key-generation.test.mjs(1 hunks)packages/wasm-sdk/test/voting-contested-resources.test.mjs(0 hunks)
💤 Files with no reviewable changes (1)
- packages/wasm-sdk/test/voting-contested-resources.test.mjs
🧰 Additional context used
📓 Path-based instructions (2)
packages/wasm-sdk/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Files:
packages/wasm-sdk/test/key-generation.test.mjs
packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Place all source packages under packages/* (JS/TS packages and Rust crates)
Files:
packages/wasm-sdk/test/key-generation.test.mjs
🧠 Learnings (4)
📚 Learning: 2025-07-28T20:00:24.323Z
Learnt from: QuantumExplorer
PR: dashpay/platform#2711
File: packages/wasm-sdk/docs.html:2359-2383
Timestamp: 2025-07-28T20:00:24.323Z
Learning: In packages/wasm-sdk/docs.html, QuantumExplorer confirmed that placeholder private keys in documentation examples are acceptable as they are not real keys, though field name accuracy for the SDK API should still be maintained.
Applied to files:
packages/wasm-sdk/test/key-generation.test.mjs
📚 Learning: 2025-07-28T20:04:48.458Z
Learnt from: QuantumExplorer
PR: dashpay/platform#2711
File: packages/wasm-sdk/index.html:4360-4416
Timestamp: 2025-07-28T20:04:48.458Z
Learning: In packages/wasm-sdk, the wallet helper `derive_key_from_seed_with_path` (Rust function in src/wallet/key_derivation.rs) is synchronous; its JS wrapper returns a value immediately, so `await` is unnecessary.
Applied to files:
packages/wasm-sdk/test/key-generation.test.mjs
📚 Learning: 2025-09-07T22:18:50.851Z
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T22:18:50.851Z
Learning: Applies to packages/wasm-sdk/** : Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Applied to files:
.github/workflows/wasm-sdk-build.yml
📚 Learning: 2025-07-31T18:15:51.813Z
Learnt from: thephez
PR: dashpay/platform#2715
File: packages/wasm-sdk/test/ui-automation/run-ui-tests.sh:6-286
Timestamp: 2025-07-31T18:15:51.813Z
Learning: In packages/wasm-sdk/test/ui-automation/run-ui-tests.sh, the script doesn't require cleanup handling because it relies on Playwright's built-in webServer configuration to manage the HTTP server lifecycle automatically. The script only runs foreground processes through npm/Playwright commands.
Applied to files:
.github/workflows/wasm-sdk-build.yml
🧬 Code graph analysis (1)
packages/wasm-sdk/test/key-generation.test.mjs (2)
packages/wasm-sdk/test/dashpay-contact-keys.test.mjs (2)
test(48-57)wasmSdk(22-22)packages/wasm-sdk/test/state-transitions.test.mjs (1)
TEST_MNEMONIC(59-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (2)
packages/wasm-sdk/test/key-generation.test.mjs (1)
361-363: Nice, focused section for child key derivation.Clear separation improves readability and future maintenance.
.github/workflows/wasm-sdk-build.yml (1)
209-209: Good: pipefail ensures CI fails on failing tests even with tee.This directly addresses the false-green behavior.
Replaces hardcoded Binaryen configuration with environment variables for better maintainability. Adds SHA256 checksum verification for security. Now version updates only require changing values at the job level. - Add BINARYEN_VERSION, BINARYEN_ARCH, and BINARYEN_SHA256 env vars - Update all references to use environment variables - Add checksum verification before extraction 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/wasm-sdk-build.yml (4)
42-45: Binaryen pin + checksum looks good; consider minor portability/traceability tweaks.
- Add BINARYEN_OS to avoid hardcoding "-linux" in URLs if this job ever runs on macOS/Windows.
- Add a brief comment noting the origin of BINARYEN_SHA256 (how it was generated).
Using BINARYEN_OS:
env: BINARYEN_VERSION: version_124 BINARYEN_ARCH: x86_64 BINARYEN_SHA256: 0290c3779fedf592b8da0ded3032ff55c41a2b7bfa2d6bf7b7bac6f0e6e28963 + BINARYEN_OS: linux # asset suffix; keeps URL portable if we matrix OS laterAnd URL change shown below in Lines 119-123 comment.
Note: Per team learnings, if any SDK-facing flags/APIs changed as part of these updates, sync WASM SDK docs (generate_docs.py). I can help verify.
110-112: Cache key/path align with versioning; optional cache-bust by checksum.Append the SHA to the cache key to avoid serving a corrupted cache if the upstream asset is ever re-published under the same tag.
- key: binaryen/${{ env.BINARYEN_VERSION }}/${{ runner.os }}/${{ env.BINARYEN_ARCH }} + key: binaryen/${{ env.BINARYEN_VERSION }}/${{ runner.os }}/${{ env.BINARYEN_ARCH }}/${{ env.BINARYEN_SHA256 }}
119-123: Harden download/extract: retries + optional cleanup; make URL OS-parameterized.
- Add curl retries to reduce flaky network failures.
- Parameterize OS suffix.
- Remove tarball after extraction.
- curl -fsSL -o /tmp/binaryen.tar.gz \ - "https://github.com/WebAssembly/binaryen/releases/download/${BINARYEN_VERSION}/binaryen-${BINARYEN_VERSION}-${BINARYEN_ARCH}-linux.tar.gz" + curl -fsSL --retry 3 --retry-delay 2 -o /tmp/binaryen.tar.gz \ + "https://github.com/WebAssembly/binaryen/releases/download/${BINARYEN_VERSION}/binaryen-${BINARYEN_VERSION}-${BINARYEN_ARCH}-${BINARYEN_OS}.tar.gz" # Verify checksum for security echo "${BINARYEN_SHA256} /tmp/binaryen.tar.gz" | sha256sum -c - tar -xzf /tmp/binaryen.tar.gz -C "${HOME}/.cache/binaryen" mv "${HOME}/.cache/binaryen/binaryen-${BINARYEN_VERSION}" "${HOME}/.cache/binaryen/${BINARYEN_VERSION}" + rm -f /tmp/binaryen.tar.gz
215-215: pipefail ensures CI fails on test failures; consider stricter shell options.Use -euo pipefail so unset vars also fail fast.
- set -o pipefail && node test/run-all-tests.mjs | tee test-output.log + set -euo pipefail + node test/run-all-tests.mjs | tee test-output.logOptionally cache npm deps to speed repeats:
- uses: actions/setup-node@v4 with: node-version: '20' + cache: 'npm' + cache-dependency-path: packages/wasm-sdk/test/package-lock.json
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/wasm-sdk-build.yml(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-07T22:18:50.851Z
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T22:18:50.851Z
Learning: Applies to packages/wasm-sdk/** : Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Applied to files:
.github/workflows/wasm-sdk-build.yml
📚 Learning: 2025-07-31T18:15:51.813Z
Learnt from: thephez
PR: dashpay/platform#2715
File: packages/wasm-sdk/test/ui-automation/run-ui-tests.sh:6-286
Timestamp: 2025-07-31T18:15:51.813Z
Learning: In packages/wasm-sdk/test/ui-automation/run-ui-tests.sh, the script doesn't require cleanup handling because it relies on Playwright's built-in webServer configuration to manage the HTTP server lifecycle automatically. The script only runs foreground processes through npm/Playwright commands.
Applied to files:
.github/workflows/wasm-sdk-build.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build JS packages / Build JS
- GitHub Check: build-wasm-sdk
🔇 Additional comments (1)
.github/workflows/wasm-sdk-build.yml (1)
127-127: PATH export is correct and scoped for subsequent steps.No issues.
|
@QuantumExplorer would you prefer I revert the workflow changes and just leave the test fixes in this or is it ok as-is? |
|
Reverted the CI commits that weren't strictly necessary to minimize changes in the PR. |
|
@shumkov could you review this one. |
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.
Issue being fixed or feature implemented
This PR fixes several tests that were failing due to recent changes and two CI/CD issues in the WASM SDK build workflow. The workflow was reporting success even when tests failed, and redundant cache save steps were causing warning messages during builds. It also switching wasm-opt back to latest (v124).
What was done?
actions/cache/save@v4steps for protoc and wasm-opt that were causing "Path Validation Error" warnings (introduced in feat(sdk): epic: rs-sdk-ffi and ios support #2756)set -o pipefailto preserve exit codes when piping test output throughtee, ensuring GitHub Actions properly fails when tests failget_contested_resourcestest to handle expected responses correctlyHow Has This Been Tested?
Locally / CI
Breaking Changes
None
Checklist:
any
For repository code-owners and collaborators only
Summary by CodeRabbit
Refactor
Tests
Chores