test: move business-rule coverage off-node (ENG-470)#515
Merged
peer2f00l merged 6 commits intoJul 20, 2026
Merged
Conversation
Move node-backed tests that only assert pure logic into fast off-node tests, keeping on-node only genuine contract-boundary crossings. universal-account: - universal_account.rs: 68 -> 10 node cases. The pure ExecuteArgs::verify table already covers all signature formats x every param/domain/payload mismatch (and env::ed25519_verify/sha256 run natively in unit tests), so the 7-signer x 3-mode grids re-proved pure logic on-chain. Keep one happy-path per host-crypto family (passkey/ed25519_raw/eip191), a constructor-execute test, and the 5 boundary singles. Delete skip_nonce / reuse_nonce (subsumed by failed_execute / replayed_nonce + the pure table); drop the `migrated` dimension (covered by migration.rs). - migration.rs: replace 12 version-guard node cases with 6 pure #[should_panic] tests in state::migration::tests (byte-exact messages), keeping the one on-chain-only case (bugged-0.4.0 borsh deserialize). vault: delete 3 happy_path.rs node dupes (duplicate supply queue, and the two fee-cap rejections) whose in-process src/tests.rs versions are strict supersets. Verified: fmt/clippy clean, 6 pure guard tests pass + mutate-to-fail confirmed, node subset 40/40 green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…f-node (ENG-470) Continue the pure-unit extraction: move node-backed tests that only assert pure functions into fast off-node #[test]s, keeping on-node only genuine contract-boundary crossings. - common: pure minimum_acceptable_liquidation_amount rstest (liquidation-math prep; liquidatable_collateral already covered). - vault: 2 pure set_fees conversions in src/tests.rs (decrease-applies- immediately, growth-rate accept+persist) replacing node fee_decrease_immediate and set_fees_accepts_max_total_assets_growth_rate. - relayer: extract validate_market_ids (generic hasher + iterator param) shared by both price handlers; 3 node rejection tests -> 3 pure route::tests. - funding-bridge: pure App::new tests + DRY Args::test_valid(); drop node test_app_initialization and test_near_handler_dry_run (dry-run covered by treasury.rs test_send_tokens_dry_run). - proxy-oracle: init_writes_current_state_version async->pure testing_env!. Verified: cargo fmt --all --check clean, clippy -D warnings clean on touched crates, all pure tests pass, mutate-to-fail confirmed each extraction, node test targets compile. Market suite left untouched: it was already curated by the ENG-388 rewrite (pure math already off-node, integration tests deliberately keep only node-worthy flows), so the ticket's ~55-extractable estimate no longer holds. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
A second review flagged that deleting both node fee tests left no on-node coverage of the vault_set_fees/vault_get_fees path — the Fees<SU128> wire conversion, gateway dispatch, and deployed-state persistence. Per ENG-470's "keep one boundary smoke per flow", restore a single consolidated smoke that sets a fee decrease + a growth-rate cap through the gateway and reads them back. The immediate-vs-timelocked decision stays covered off-node by the pure set_fees_* tests in src/tests.rs. Verified: passes on-node (fee_changes_round_trip_through_the_gateway, 26.5s). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
A second, more thorough review found four extractions that dropped genuine boundary coverage (not equivalent to the pure replacements). Restore the minimum on-node coverage per ENG-470's "keep one boundary smoke per flow": - universal-account: re-add eip712 + sep53 to execute_advances_nonce so all five signing formats init through the deployed `new` ABI, persist, list, and execute on-node (verify stays pure). eip712 had no remaining node coverage. - universal-account: execute_on_create_runs_constructor_transactions now asserts `new` installs the controller key (nonce 0) alongside the constructor batch — previously it only checked the batch ran. - funding-bridge: restore test_near_handler_dry_run, which proves the safety invariant that a dry-run submits no real transfer (balance unchanged) — the pure test only checks the returned marker. - vault: restore one narrow supply_queue_mustnt_have_duplicates gateway smoke (curator auth + account->MarketId resolution + duplicate panic); the exhaustive rule stays pure in src/tests.rs. Declined (documented in the plan): 0.2.0 migrate-then-execute (covered in combination by from_0_2_0 state + from_0_4_0_unbrick_v1 V2 execution) and relayer handler-wiring tests (App::new needs live Postgres+gateway = a node test, contra ENG-470; glue is trivial and still hit by success-path tests). Verified: all four pass on-node; cargo fmt --all --check clean. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummaryMoves deterministic business-rule coverage for market logic, universal-account migrations, funding-bridge config/init, and relayer market-ID validation from sandbox-style tests into pure unit tests, while keeping on-node boundary smoke coverage for treasury/vault/gateway safety and legacy state-shape failures. Key changes:
Review focus
WalkthroughThe pull request expands market, contract, vault, funding-bridge, and relayer test coverage. It centralizes relayer market validation, refactors universal-account setup around constructor execution, adds migration regressions, and moves funding-bridge initialization checks into unit tests. ChangesMarket liquidation validation
Contract state and execution tests
Vault governance test coverage
Funding bridge initialization tests
Relayer market validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/funding-bridge/src/config.rs (1)
165-196: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCompile this fixture only for tests.
Args::test_valid()is test-only, but thisimplis compiled in non-test builds. That keeps a test private-key literal andunwrap()calls in production Rust, and a future fixture typo could become a panic if the helper is reused. Gate the method or impl with#[cfg(test)].As per coding guidelines, Rust code must not introduce
unwrap()in non-test code unless a documented invariant makes it safe.Proposed fix
-impl Args { +#[cfg(test)] +impl Args {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/funding-bridge/src/config.rs` around lines 165 - 196, Gate the test_valid method in impl Args with #[cfg(test)] so the fixture, private-key literal, and unwrap calls are compiled only for test builds. Keep the existing fixture behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/src/market/configuration.rs`:
- Around line 647-668: The test helper price_pair currently hardcodes identical
exponents and decimal arguments, so it cannot exercise decimal normalization.
Update price_pair and its callers to accept distinct collateral and borrow
decimal/exponent values, then add assertions covering unequal scales while
preserving the existing equal-scale cases and checking arithmetic edge behavior
in the related common tests.
In `@service/funding-bridge/tests/tests.rs`:
- Around line 162-165: Strengthen the dry-run accounting assertion in the node
smoke test by capturing both treasury and recipient balances before invoking the
transfer and comparing both after the call. Update the existing dry-run test
flow around the balance checks so it verifies neither account changes,
preserving the end-to-end no-transfer invariant.
In `@service/relayer/tests/relayer.rs`:
- Around line 586-588: Restore handler-level regression coverage for both
/update_prices and /get_market_prices, verifying each route invokes
validate_market_ids and returns the expected SimpleResponse::Rejected reason for
empty or unknown markets. Keep the tests lightweight and retain the existing
pure validation tests.
---
Outside diff comments:
In `@service/funding-bridge/src/config.rs`:
- Around line 165-196: Gate the test_valid method in impl Args with #[cfg(test)]
so the fixture, private-key literal, and unwrap calls are compiled only for test
builds. Keep the existing fixture behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6eccd6c1-2cf1-4366-8bab-d1a9cf48673e
📒 Files selected for processing (15)
common/src/market/configuration.rscontract/proxy-oracle/near/contract/tests/migrate_v0.rscontract/universal-account/tests/migration.rscontract/universal-account/tests/universal_account.rscontract/vault/near/src/tests.rscontract/vault/near/tests/governance.rscontract/vault/near/tests/happy_path.rsservice/funding-bridge/src/app.rsservice/funding-bridge/src/config.rsservice/funding-bridge/tests/tests.rsservice/relayer/src/route/get_market_prices.rsservice/relayer/src/route/mod.rsservice/relayer/src/route/update_prices.rsservice/relayer/tests/relayer.rsuniversal-account/src/state/migration.rs
…nit-extraction-move-business-rule-tests-off-the-node
peer2f00l
deleted the
feature/eng-470-pure-unit-extraction-move-business-rule-tests-off-the-node
branch
July 20, 2026 22:27
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
1 participant
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.
Summary
Test plan
Linear: https://linear.app/templar-protocol/issue/ENG-470/pure-unit-extraction-move-business-rule-tests-off-the-node
This change is