Skip to content

feat: hyperevm composer [SV-1413]#296

Open
supervaulter wants to merge 2 commits intodevfrom
feat/hyperevm-composer-sv-1413
Open

feat: hyperevm composer [SV-1413]#296
supervaulter wants to merge 2 commits intodevfrom
feat/hyperevm-composer-sv-1413

Conversation

@supervaulter
Copy link
Collaborator

@supervaulter supervaulter commented Feb 25, 2026

Note

Medium Risk
Deploys new cross-chain compose infrastructure and recovery paths; while largely additive, mistakes in deployment parameters (core index/decimals) or operational wiring could misroute funds or affect bridging behavior.

Overview
Implements UpHyperLiquidComposer, a thin RecoverableComposer-based wrapper that enables LayerZero compose messages to forward bridged UP into HyperCore spot trading with MPC-gated emergency recovery.

Adds deterministic HyperEVM deployment tooling (DeployUpComposer.s.sol + deploy_up_composer.sh) including address precomputation, idempotent CREATE2 deploy, and JSON export, with deployment blocked until CORE_INDEX_ID/decimal diff are finalized.

Introduces extensive unit + HyperEVM-fork integration tests (with precompile mocks) covering happy-path compose flows (ERC20 and native value), failed-message/refund behavior, and recovery access control. Also adds operational docs/specs including explorer calldata for ETH/Base→HyperEVM wiring and HyperLiquid Composer integration guidance.

Written by Cursor Bugbot for commit 53b690e. This will update automatically on new commits. Configure here.

@linear
Copy link

linear bot commented Feb 25, 2026

@claude
Copy link

claude bot commented Feb 25, 2026

PR Review: feat: hyperevm composer [SV-1413]

Overall this is a clean implementation. The main contract is appropriately minimal — just a thin wrapper over the upstream RecoverableComposer with a well-structured constructor. The test suite is thorough. A few issues worth addressing before deployment.


Issues

1. Missing deployment guard for ASSET_DECIMAL_DIFF (Medium)

script/DeployUpComposer.s.sol correctly guards against CORE_INDEX_ID == 0, but there's no equivalent check for ASSET_DECIMAL_DIFF. The current placeholder value is 0, which happens to also be a valid production value (if Core uses 18 decimals). If Core uses 8 decimals and someone deploys without updating this constant, tokens would be scaled 1:1 instead of the required 10x scaling — silently, with no revert.

Suggestion: Add a deployment checklist comment explicitly requiring this to be verified, or add an env-var-driven override so the value cannot remain at the wrong default.

// Current — no guard possible since 0 is valid
int8 internal constant ASSET_DECIMAL_DIFF = 0; // TODO: Replace with actual value

At minimum, add a comment like: "Verified with Step 3 team: Core uses 18 decimals, assetDecimalDiff = 0 is correct" once confirmed, so reviewers and auditors know it was intentional.


2. Integration test: CORE_WRITER precompile not mocked (Minor)

test/integration/Up/UpHyperLiquidComposer.t.sol etches mock code onto SPOT_BALANCE_PRECOMPILE and CORE_USER_EXISTS_PRECOMPILE, but leaves CORE_WRITER (0x3333...) untouched on the fork. Several tests use vm.expectEmit(CORE_WRITER) expecting a specific ICoreWriter.RawAction event.

On a HyperEVM fork the CoreWriter at that address may or may not correctly emit EVM-observable events (it's an L1 precompile). The unit tests solve this correctly by etching a CoreWriterMock. If those integration test assertions ever start silently passing without actually calling the real CoreWriter, the event-based verification gives false confidence.

Consider either etching the same CoreWriterMock in the integration setup, or adding an assertion on a side-effect (e.g. ERC20 balance at the bridge) in addition to the event check as the primary verification.


3. Weak revert assertion in gas test (Minor)

test/unit/up/UpHyperLiquidComposer.t.sol line ~899:

vm.expectRevert();
vm.prank(endpoint);
composer.lzCompose{ gas: 150_000 }(address(oft), bytes32(0), composerMsg, address(0), "");

Using a bare vm.expectRevert() means any revert — including an unrelated bug — would make this test pass. Prefer vm.expectRevert(IHyperLiquidComposer.InsufficientGas.selector) (or whatever the specific error is) to validate the correct failure mode.


4. specs/ directory contains AI-generated research artifacts (Nit)

The PR adds 7 markdown files under specs/hyperliquid-composer/ that read as AI research/spec-generation artifacts, including interview-notes.md which names "Interviewer: Claude Code". These are valuable during development but are unlikely to be useful to future contributors in the repo.

Consider keeping only the user-facing docs (docs/hyperliquid-composer-spec.md, docs/HyperEVM_Configuration_Calldata.md) and dropping the internal specs from the PR, or moving them to a separate internal wiki/Notion.


Good

  • Minimal contractUpHyperLiquidComposer.sol is 38 lines and does exactly what's needed. The dual parent constructor order (RecoverableComposer before HyperLiquidComposer) is correct and documented.
  • Comprehensive test coverage — unit tests cover constructor validation, happy paths, refund paths, and all access control cases. Integration tests add fork-level validation with real contract addresses.
  • Graceful fork skip — the try vm.createSelectFork(...) {} catch { vm.skip(true); } pattern correctly avoids CI failures when the RPC is unavailable.
  • CORE_INDEX_ID guard — the require before deployment is a good safety net to prevent deploying with the placeholder value.
  • RecoverableComposer — the choice to extend this rather than the base composer is the right call given the risks of tokens getting stuck at the asset bridge.
  • Deployment script pattern is consistent with DeployUpOFT.s.sol (env/salt/export structure).

One question

After deploying the Composer, does the UP OFT on HyperEVM need setComposer(composerAddress) to be called? Some OFT variants require this to activate the compose pathway. The deployment script post-deployment steps mention activating on HyperCore but don't list this step. Worth confirming with the LZ integration before mainnet deployment.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

console2.log("Chain ID:", block.chainid);
console2.log("UP OFT:", UP_OFT_HYPEREVM);
console2.log("Core Index ID:", CORE_INDEX_ID);
console2.log("Asset Decimal Diff:", uint8(ASSET_DECIMAL_DIFF));
Copy link

Choose a reason for hiding this comment

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

Casting int8 to uint8 produces misleading deployment log

Medium Severity

ASSET_DECIMAL_DIFF is declared as int8 with a valid range of [-2, 18], but the deployment log casts it to uint8. When this TODO placeholder is updated to a negative value (e.g., -2), the explicit uint8 cast will silently reinterpret via two's complement — logging 254 instead of -2. During a critical mainnet deployment, this misleading output could prevent operators from catching a misconfigured parameter.

Fix in Cursor Fix in Web

If you need to regenerate this calldata (e.g., for different addresses), run:

```bash
cd /Users/cosming/1.Coding/Superform/v2-periphery
Copy link

Choose a reason for hiding this comment

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

Local filesystem path committed in documentation

Low Severity

The "Regenerating Calldata" section contains an absolute local filesystem path (/Users/cosming/1.Coding/Superform/v2-periphery) that exposes a developer's machine username and directory structure. This is not useful for other developers and leaks personal info. It can be replaced with a relative path or a generic instruction.

Fix in Cursor Fix in Web

@claude
Copy link

claude bot commented Feb 25, 2026

PR Review: feat: hyperevm composer [SV-1413]

Note: A prior automated review was posted earlier today covering the ASSET_DECIMAL_DIFF guard, CoreWriter mock gap in integration tests, bare vm.expectRevert() in the gas test, and the specs/ directory question. This comment adds findings from a second pass.


Additional Findings

1. Hardcoded developer-local path in docs (Minor)

docs/HyperEVM_Configuration_Calldata.md line 261 contains an absolute path from the author's machine:

cd /Users/cosming/1.Coding/Superform/v2-periphery

Replace with a generic instruction such as:

# from the repo root
forge script script/GenerateHyperEVMCalldata.s.sol -vvv

2. Integration test lifecycle is incomplete for refundToSrc (Minor)

test_failedMessageThenRefund verifies that a failed compose stores the message correctly, but stops there. The unit test suite does test the full refundToSrc success path, but the integration test with the real OFT never exercises it. Since refundToSrc calls the real OFT.send() on-chain (which requires fee quoting, approved balance, etc.), a fork-level test covering this path would catch integration issues that the mock-based unit test cannot.


3. 1Password vault ID embedded in shell script (Nit)

script/run/deploy_up_composer.sh line 96:

op read op://5ylebqljbh3x6zomdxi3qd7tsa/HYPEREVM_RPC_URL/credential

The vault ID (5ylebqljbh3x6zomdxi3qd7tsa) is specific to the team's 1Password account. This is a public repo — while the vault ID alone is not a secret, having it hardcoded makes onboarding harder. Consider referencing by vault name and item name instead (e.g. op://Engineering/HYPEREVM_RPC_URL/credential) or just falling through to an env var. The existing fallback makes this low risk.


Confirmations from second pass

  • Salt convention ("TEST1.0.0" for env == 0 / prod): consistent with DeployUpOFT.s.sol — not a bug, it is the established repo convention.
  • MNEMONIC constant: only reachable via env == 1 (local/test mode), which the shell script never passes. Benign dead code.
  • computeAddress / exportAddresses skipping the CORE_INDEX_ID != 0 guard: intentional — pre-computing the deterministic address before Step 3 is a valid use case.

The main contract and test suite are solid. All items above are minor/nit-level. The primary blocker before mainnet deployment remains confirming CORE_INDEX_ID and ASSET_DECIMAL_DIFF with the Step 3 team, verifying whether setComposer needs to be called on the UP OFT (raised in the earlier review), and completing the post-deployment HyperCore activation steps.

@codecov
Copy link

codecov bot commented Feb 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant