feat(contracts): add pyth pro verifier#2079
Conversation
Adds a query-only CosmWasm verifier for upgraded Pyth Core payloads. The verifier preserves the VerifyVAA query shape used by pyth.wasm while checking the Pyth Pro router set, expected emitter, and 3-of-5 quorum. Includes a live upgraded Hermes AKT/USD fixture test proving the existing PNAU parser still accepts the upgraded payload format. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughChangesPyth router verifier migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Hermes
participant PythContract
participant RouterVerifier
participant Oracle
Hermes->>PythContract: submit PNAU accumulator update
PythContract->>PythContract: verify Merkle proof
PythContract->>RouterVerifier: verify embedded VAA
RouterVerifier-->>PythContract: verified VAA
PythContract->>Oracle: relay parsed Pyth price
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contracts/pyth-pro-verifier/src/contract.rs (1)
93-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSignature verification logic is sound.
The quorum threshold, strictly-increasing index enforcement, bounds checking, ECDSA recovery, and router address comparison are all correct. The
router_addressderivation follows standard Ethereum address format (Keccak256 of uncompressed EC point minus prefix, last 20 bytes).One minor semantic issue at lines 116-118: when an individual router index exceeds
config.routers.len(),TooManySignaturesis returned. This is misleading — the count is valid but the index is out of bounds. Consider a dedicatedInvalidRouterIndexvariant or reusingInvalidRouterSetIndex.🤖 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 `@contracts/pyth-pro-verifier/src/contract.rs` around lines 93 - 142, The per-signature router index bounds check in verify_router_signatures is returning TooManySignatures when an individual index is out of range, which is the wrong semantic error. Update that branch to use a dedicated invalid-index error such as InvalidRouterIndex or the existing InvalidRouterSetIndex variant, and keep the rest of the signature validation flow unchanged.contracts/pyth-pro-verifier/src/testing.rs (1)
234-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest helpers are well-structured and consistent with contract logic.
body_hash(double Keccak256) matchesparse_vaa's hash computation, andsign_hash(sign_prehash_recoverable) is the correct counterpart torecover_from_prehashin the contract.address_from_keycorrectly implements Ethereum-style address derivation. The separation ofsigned_vaa(index→key mapping) vssigned_vaa_with_keys(explicit keys) enables testing both valid and adversarial signer configurations.One minor gap: no test exercises the
TooManySignaturespath (signer_count > routers.len()). Consider adding a case with 6 signers against the 5-router config.🧪 Suggested test for TooManySignatures
#[test] fn rejects_too_many_signatures() { let keys = router_keys(); let deps = setup(&keys); let vaa = signed_vaa( &keys, &[0, 1, 2, 3, 4, 0], // 6 signers, but index 0 is reused — order check catches first ROUTER_SET_INDEX, EMITTER_CHAIN, EMITTER_ADDRESS, vec![], ); let err = query( deps.as_ref(), mock_env(), QueryMsg::VerifyVAA { vaa: Binary::from(vaa), block_time: 0, }, ) .unwrap_err(); // With 6 signers > 5 routers, expect TooManySignatures assert!(err.to_string().contains("TooManySignatures")); }Note: the signer indices must be strictly increasing to reach the
signer_count > config.routers.len()check, so use indices like[0, 1, 2, 3, 4, 5]with 6 distinct keys.🤖 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 `@contracts/pyth-pro-verifier/src/testing.rs` around lines 234 - 379, Add a test in testing.rs that covers the TooManySignatures branch in the verification flow. Use the existing helpers setup, router_keys, signed_vaa_with_keys, and query to build a VAA with 6 distinct signatures against the 5-router config so it reaches the signer_count > config.routers.len() check in the contract’s VerifyVAA path. Assert the query fails with TooManySignatures, and make sure the signer indexes are strictly increasing so the failure comes from signature count rather than ordering.
🤖 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.
Nitpick comments:
In `@contracts/pyth-pro-verifier/src/contract.rs`:
- Around line 93-142: The per-signature router index bounds check in
verify_router_signatures is returning TooManySignatures when an individual index
is out of range, which is the wrong semantic error. Update that branch to use a
dedicated invalid-index error such as InvalidRouterIndex or the existing
InvalidRouterSetIndex variant, and keep the rest of the signature validation
flow unchanged.
In `@contracts/pyth-pro-verifier/src/testing.rs`:
- Around line 234-379: Add a test in testing.rs that covers the
TooManySignatures branch in the verification flow. Use the existing helpers
setup, router_keys, signed_vaa_with_keys, and query to build a VAA with 6
distinct signatures against the 5-router config so it reaches the signer_count >
config.routers.len() check in the contract’s VerifyVAA path. Assert the query
fails with TooManySignatures, and make sure the signer indexes are strictly
increasing so the failure comes from signature count rather than ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3323207f-57a0-4265-8717-3ccf6926765c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlcontracts/pyth-pro-verifier/Cargo.tomlcontracts/pyth-pro-verifier/src/contract.rscontracts/pyth-pro-verifier/src/error.rscontracts/pyth-pro-verifier/src/lib.rscontracts/pyth-pro-verifier/src/msg.rscontracts/pyth-pro-verifier/src/state.rscontracts/pyth-pro-verifier/src/testing.rscontracts/pyth-pro-verifier/src/vaa.rscontracts/pyth/src/accumulator.rs
Reuses the existing Wormhole VAA parser in the Pyth Pro verifier so the new contract only owns router quorum and emitter validation logic. This removes the duplicate parser added in the initial implementation. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
Instantiate the Pyth Pro verifier during the v2.1.0 upgrade and migrate the existing Pyth contract to use that verifier address. Local init now follows the same contract path and skips the old Wormhole deployment. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
Move upgraded Pyth router quorum verification into the existing pyth contract so Akash accepts router-signed PNAU payloads from upgraded Hermes. This removes the separate verifier contract and the legacy Wormhole smart-query path. Pyth contracts are now deployed directly with router verifier config instead of being embedded in the v2.1.0 chain upgrade. BREAKING CHANGE: pyth instantiate and config messages now require router_verifier config instead of wormhole_contract and data_sources. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@contracts/pyth/src/contract.rs`:
- Around line 366-369: Update migrate to read CONFIG using a legacy config type
that omits router_verifier before deserializing the new Config, then construct
the current Config with the parsed router configuration and save it. Preserve
handling for already-migrated state as appropriate, and add a migration test
that seeds legacy-serialized CONFIG data and verifies successful migration to
the new schema.
In `@contracts/pyth/src/state.rs`:
- Around line 37-48: The migration must support legacy Config records that lack
router_verifier. Add a dedicated LegacyConfig matching the old schema, update
the migration in contract.rs to load LegacyConfig, construct Config with the
supplied router settings, and persist the converted configuration atomically
rather than deserializing the legacy record as Config.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 873fe62d-aafd-42ec-ade5-5e6ae0361813
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
_run/init.shcontracts/pyth/Cargo.tomlcontracts/pyth/src/accumulator.rscontracts/pyth/src/contract.rscontracts/pyth/src/error.rscontracts/pyth/src/integration_tests.rscontracts/pyth/src/lib.rscontracts/pyth/src/msg.rscontracts/pyth/src/pyth.rscontracts/pyth/src/router.rscontracts/pyth/src/state.rscontracts/pyth/src/wormhole.rsscript/wasm2go.shtests/e2e/pyth_contract_test.goupgrades/software/v2.1.0/contracts.goupgrades/software/v2.1.0/upgrade.go
💤 Files with no reviewable changes (2)
- script/wasm2go.sh
- upgrades/software/v2.1.0/upgrade.go
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/pyth/src/accumulator.rs
| if let Some(router_config) = msg.router_verifier { | ||
| let mut config = CONFIG.load(deps.storage)?; | ||
| config.router_verifier = router::parse_config(router_config)?; | ||
| CONFIG.save(deps.storage, &config)?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the current state definitions before comparing the pre-change schema.
ast-grep outline contracts/pyth/src/state.rs --items all
# Inspect the parent version's state/config definitions and migration-related diff.
git show HEAD^:contracts/pyth/src/state.rs | sed -n '1,180p'
git diff --unified=80 HEAD^ HEAD -- contracts/pyth/src/state.rs contracts/pyth/src/contract.rsRepository: akash-network/node
Length of output: 36765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect migration message and config response shapes.
ast-grep outline contracts/pyth/src/msg.rs --items all
# Read the relevant message definitions and any migration tests.
sed -n '1,240p' contracts/pyth/src/msg.rs
printf '\n--- tests mentioning migrate or legacy config ---\n'
rg -n "legacy|migrate|router_verifier|wormhole_contract|data_sources" contracts/pyth/src -g '!target'Repository: akash-network/node
Length of output: 8096
Migrate the legacy CONFIG before deserializing it as the new type. migrate still loads CONFIG as the new Config, but pre-router deployments were saved without router_verifier. That load will fail before parse_config runs, so the contract can’t be migrated in place. Read the legacy config shape first, rebuild and save the new Config, and add a migration test seeded with legacy-serialized state.
🤖 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 `@contracts/pyth/src/contract.rs` around lines 366 - 369, Update migrate to
read CONFIG using a legacy config type that omits router_verifier before
deserializing the new Config, then construct the current Config with the parsed
router configuration and save it. Preserve handling for already-migrated state
as appropriate, and add a migration test that seeds legacy-serialized CONFIG
data and verifies successful migration to the new schema.
| pub struct Config { | ||
| /// Admin address that can update contract settings | ||
| pub admin: Addr, | ||
| /// Wormhole contract address for VAA verification | ||
| pub wormhole_contract: Addr, | ||
| /// Upgraded Pyth router verifier configuration | ||
| pub router_verifier: RouterVerifierConfig, | ||
| /// Fee required to update the price feed (in Uint256 for CosmWasm 3.x) | ||
| pub update_fee: Uint256, | ||
| /// Pyth price feed ID for AKT/USD | ||
| pub price_feed_id: String, | ||
| /// Default data ID for price submissions (denom + base_denom) | ||
| pub default_data_id: DataID, | ||
| /// Valid Pyth data sources (emitter chain + address pairs) | ||
| pub data_sources: Vec<DataSource>, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Migrate the legacy storage schema before loading the new Config.
Existing records lack the required router_verifier field. The migration in contracts/pyth/src/contract.rs loads them directly as this new type, so deserialization fails; with router_verifier: None, migration succeeds without converting state and leaves the contract unreadable.
Load a dedicated LegacyConfig, construct the new Config using the supplied router settings, and save it atomically. This currently blocks upgrading the existing Pyth contract.
🤖 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 `@contracts/pyth/src/state.rs` around lines 37 - 48, The migration must support
legacy Config records that lack router_verifier. Add a dedicated LegacyConfig
matching the old schema, update the migration in contract.rs to load
LegacyConfig, construct Config with the supplied router settings, and persist
the converted configuration atomically rather than deserializing the legacy
record as Config.
The router verifier contract is intended for fresh deployment, so keeping a migrate entrypoint implied an unsupported in-place legacy state conversion path. Removing it avoids a broken migration surface and aligns the contract with the deployment plan. Router signature validation now reports out-of-range signer indexes separately from payloads that carry more signatures than the configured router set. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
1 participant
Summary
Adds a query-only CosmWasm verifier for upgraded Pyth Core payloads. The upgraded flow replaces Wormhole guardian signature verification with the Pyth router signer model, so Akash needs a verifier that checks the router set, expected emitter, and 3-of-5 quorum.
The verifier preserves the existing
VerifyVAA { vaa, block_time }query shape used bypyth.wasm. It reuses the existing VAA envelope parser, but signature verification is router-quorum based, not Wormhole guardian based.This also wires the contract side into the v2.1.0 upgrade: the upgrade embeds and instantiates the verifier, then migrates the existing Pyth contract to use the new verifier address. Local init now follows the same path and no longer deploys the old Wormhole contract for Pyth.
Hermes/API-key changes are intentionally out of scope for this PR.
Validation
bash -n _run/init.sh script/wasm2go.shcargo test -p pythcargo test -p pyth-pro-verifiercargo clippy -p pyth-pro-verifier --all-targets -- -D warningscargo build --release --target wasm32-unknown-unknown -p wormhole -p pyth -p pyth-pro-verifierGOWORK=off go test ./upgrades/...git diff --check