test(precompiles): add golden tests for factory V1 (BOP-424)#4015
Conversation
✅ Heimdall Review Status
|
7318c75 to
b67edca
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #[test] | ||
| fn golden_create_reverts_malformed_params() { | ||
| let mut s = fresh(); | ||
| let (rev, _bytes) = call_factory( | ||
| &mut s, | ||
| CREATOR, | ||
| create_call( | ||
| IB20Factory::B20Variant::ASSET, | ||
| SALT, | ||
| Bytes::from(vec![0xaa_u8, 0xbb, 0xcc]), | ||
| vec![], | ||
| ), | ||
| ); | ||
| assert!(rev); | ||
| } |
There was a problem hiding this comment.
nit: For a golden test that pins exact behavior, discarding the revert bytes (_bytes) means the specific revert selector/payload isn't pinned. If the factory changes which error it returns for malformed params, this test would still pass. Same applies to golden_create_reverts_when_not_activated below. Consider asserting the exact revert bytes (or at minimum the error selector) to match the thoroughness of the other revert tests.
b67edca to
bd96a39
Compare
| StorageCtx::enter(&mut s, |ctx| { | ||
| B20FactoryStorage::new(ctx).dispatch(ctx, &calldata, BaseUpgrade::Beryl) | ||
| }) | ||
| .expect("gas-footprint op must succeed"); | ||
| (s.counter_sload(), s.counter_sstore(), s.counter_keccak256()) |
There was a problem hiding this comment.
The gas() helper unwraps the outer Result but doesn't assert that the dispatch actually succeeded (i.e. !output.is_revert()). If a future refactor introduces a bug that makes one of these calls revert, this test would silently measure the revert-path gas footprint rather than failing.
| StorageCtx::enter(&mut s, |ctx| { | |
| B20FactoryStorage::new(ctx).dispatch(ctx, &calldata, BaseUpgrade::Beryl) | |
| }) | |
| .expect("gas-footprint op must succeed"); | |
| (s.counter_sload(), s.counter_sstore(), s.counter_keccak256()) | |
| let out = StorageCtx::enter(&mut s, |ctx| { | |
| B20FactoryStorage::new(ctx).dispatch(ctx, &calldata, BaseUpgrade::Beryl) | |
| }) | |
| .expect("gas-footprint op must not fatally error"); | |
| assert!(!out.is_revert(), "gas-footprint op must not revert"); |
Consumer-side ABI integration notes from BasePayHi team — commenting from the consumer/dApp side. We've integrated the B20 factory precompile into BasePay (https://github.com/osr21/basepay-dapp) and want to share the ABI we derived from the spec, which aligns with what these golden tests cover: // B20 factory ABI used in BasePay (wagmi.ts)
export const B20_FACTORY_ABI = [
{
type: "function", name: "createB20",
inputs: [
{ name: "variant", type: "uint8" }, // 0 = ASSET, 1 = STABLECOIN
{ name: "salt", type: "bytes32" }, // deterministic address derivation
{ name: "params", type: "bytes" }, // ABI-encoded (name, symbol, admin, [currencyCode])
{ name: "initCalls", type: "bytes[]" }, // optional post-creation calls
],
outputs: [{ name: "tokenAddress", type: "address" }],
stateMutability: "nonpayable",
},
{
type: "function", name: "getB20Address",
inputs: [
{ name: "variant", type: "uint8" },
{ name: "deployer", type: "address" },
{ name: "salt", type: "bytes32" },
],
outputs: [{ name: "", type: "address" }],
stateMutability: "view",
},
] as const;Error guards we're surfacing to users: One thing that would help dApp developers: a canonical npm-published ABI package (or typed viem contract definition) for the B20 factory and B20 token interface, so we're not hand-writing ABIs from the spec. Is that on the roadmap alongside these golden tests? |
bd96a39 to
3e2cc5d
Compare
Approved review 4726210827 from rayyan224 is now dismissed due to new commit. Re-request for approval.
| read_stablecoin(&mut s, token, |t| { | ||
| assert_eq!(t.name().unwrap(), SC_NAME); | ||
| assert_eq!(t.symbol().unwrap(), SC_SYMBOL); | ||
| assert!(t.has_role(B20TokenRole::DefaultAdmin.id(), ADMIN).unwrap()); | ||
| }); |
There was a problem hiding this comment.
The asset golden (golden_create_asset) asserts the variant-specific field (decimals), but this stablecoin golden doesn't assert that currency was stored correctly. B20StablecoinStorage exposes currency() via StablecoinAccounting — adding an assertion here would close the gap and pin the currency write in the frozen manifest.
| read_stablecoin(&mut s, token, |t| { | |
| assert_eq!(t.name().unwrap(), SC_NAME); | |
| assert_eq!(t.symbol().unwrap(), SC_SYMBOL); | |
| assert!(t.has_role(B20TokenRole::DefaultAdmin.id(), ADMIN).unwrap()); | |
| }); | |
| read_stablecoin(&mut s, token, |t| { | |
| assert_eq!(t.name().unwrap(), SC_NAME); | |
| assert_eq!(t.symbol().unwrap(), SC_SYMBOL); | |
| assert_eq!(StablecoinAccounting::currency(t).unwrap(), CURRENCY); | |
| assert!(t.has_role(B20TokenRole::DefaultAdmin.id(), ADMIN).unwrap()); | |
| }); |
| let expected: &[(&str, (u64, u64, u64))] = &[ | ||
| ("create_asset", (3, 7, 1)), | ||
| ("create_stablecoin", (3, 6, 1)), | ||
| ("get_b20_address", (0, 0, 1)), | ||
| ("is_b20", (0, 0, 0)), | ||
| ]; |
There was a problem hiding this comment.
nit: isB20Initialized is a distinct op in the coverage checklist (line 784) but its storage-access footprint isn't pinned here. It exercises a different code path from isB20 (storage read vs. pure address-prefix check). Consider adding it for completeness:
("is_b20_initialized", (1, 0, 0)), // or whatever the actual counts are
Follow-up from BasePay integration (consumer-side review)Good coverage overall — the compile-time coverage checklist ( 1.
|
3e2cc5d to
8954b3f
Compare
Approved review 4735543548 from rayyan224 is now dismissed due to new commit. Re-request for approval.
Pins Factory V1 behavior of the B-20 precompile: token creation flows (createB20 asset + stablecoin, with/without initCalls, zero-admin), deterministic address derivation (getB20Address), prefix/initialized reads (isB20, isB20Initialized), and every validation guard (TokenAlreadyExists, UnsupportedVersion, InvalidDecimals, MissingRequiredField, InvalidCurrency, InitCallFailed, typed-revert propagation, malformed params, not-activated, NonPayable). Each case asserts returned bytes/typed reverts, the created token's state + code, emitted events (B20Created + init events), and a per-case keccak storage-hash snapshot scoped to the factory + created token; plus per-op gas footprints and a compile-time op-coverage checklist. Authored and blessed against the shipped v1.1.1 (pre-versioned) factory, then confirmed identical pins pass on the versioned (resolver-gated) structure - proving the SOLID/versioned migration (BOP-418) is behavior-preserving. ~96% line coverage of b20_factory/logic/v1.rs (remainder are defensive error edges). Test-only: new integration test + one Cargo.toml [[test]] stanza; no source/logic changes. Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
8954b3f to
1707d4a
Compare
Review SummaryTest-only PR adding 806 lines of golden/snapshot tests for Factory V1 behavior. No production code changes. No new issues found. All prior inline review comments have been addressed in the current revision:
The code is well-structured: the compile-time exhaustive-match coverage checklist ( LGTM — no blocking concerns. |
✅ base-std fork tests: all 616 passedbase/base is fully in sync with the base-std spec.
|
4 participants
What
Adds a golden/snapshot suite pinning Factory V1 behavior of the B-20 precompile (
crates/common/precompiles/tests/b20_factory_v1_golden.rs), driven through the realB20FactoryStoragedispatch entry. Covers all 4 ops and every validation guard:createB20— asset + stablecoin success (created token state/code +B20Createdevent), withinitCalls(asset & stablecoin), zero-admin (skips role grant), and guards:TokenAlreadyExists,UnsupportedVersion,InvalidDecimals,MissingRequiredField,InvalidCurrency,InitCallFailed, typed-revert propagation from an init call, malformed params, not-activated, andNonPayable.getB20Address— deterministic address derivation (asset + stablecoin).isB20/isB20Initialized— prefix + factory-initialized reads.Each case asserts exact returned bytes (or typed revert), the created token's resulting state, emitted events, and a per-case keccak storage-hash snapshot scoped to the factory + created token (excludes activation scaffolding); plus per-op storage-access gas footprints and a compile-time op-coverage checklist over
IB20FactoryCalls.Why
Baseline for the frozen-manifest check (BOP-422/BOP-424). The pins were authored and blessed against the shipped v1.1.1 (pre-versioned) factory implementation, then confirmed to pass unchanged on the versioned (resolver-gated) structure — proving the SOLID/versioned migration (BOP-418) is behavior-preserving.
Testing
~96% line coverage of
b20_factory/logic/v1.rs(remainder are defensive?error edges + the unused non-observercreate_b20alt-API). Test-only: new integration test + oneCargo.toml[[test]]stanza; no source/logic changes.