fix(protocol-devtools): treat explicitly-empty ULN config values as NIL sentinels#1944
Open
St0rmBr3w wants to merge 17 commits into
Open
fix(protocol-devtools): treat explicitly-empty ULN config values as NIL sentinels#1944St0rmBr3w wants to merge 17 commits into
St0rmBr3w wants to merge 17 commits into
Conversation
…IL sentinels An explicitly-empty ULN302 OApp config field now pins literal zero/none via the protocol NIL sentinel, while an omitted field still inherits the on-chain default: - confirmations: 0n -> NIL_CONFIRMATIONS (type(uint64).max) - optionalDVNs: [] -> NIL_DVN_COUNT (0xff), matching requiredDVNs: [] - omitted fields continue to inherit the default Adds optionalDVNCount to the Uln302UlnConfig/UlnReadUlnConfig read shapes (and requiredDVNCount to UlnReadUlnConfig) so the stored sentinel round-trips through the config diff. The on-chain read path no longer re-applies the empty->NIL mapping, keeping hasAppUlnConfig idempotent. The library-wide DEFAULT config still serializes literal values. On Solana, confirmations is encoded as a BN so the u64 NIL sentinel survives without precision loss. Covers EVM (uln302 + ulnRead) and Solana.
PR SummaryHigh Risk Overview EVM Uln302 / UlnRead gain a Integrators who used Reviewed by Cursor Bugbot for commit fb9da3b. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit fb9da3b. Configure here.
| requiredDVNs: requiredDVNs.map(addChecksum).sort(compareBytes32Ascending), | ||
| optionalDVNs: (optionalDVNs ?? []).map(addChecksum).sort(compareBytes32Ascending), | ||
| requiredDVNCount: resolvedRequiredDVNCount, | ||
| optionalDVNCount: resolvedOptionalDVNCount, |
There was a problem hiding this comment.
encodeUlnConfig ignores chain semantics
High Severity
encodeUlnConfig still runs every input through user-oriented serializeUlnConfig, so chain-shaped configs (e.g. from getAppUlnConfig or decode→encode) are mis-encoded: stored inherit confirmations of 0 becomes NIL_CONFIRMATIONS, and optionalDVNCount: 0 with optionalDVNs: [] becomes NIL_DVN_COUNT because optionalDVNCount is not honored like requiredDVNCount.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fb9da3b. Configure here.
- generator (creatUlnConfig): faithfully distinguish inherit (omit / count 0) from pin-none (emit [] or 0n for NIL), so regenerated configs no longer silently flip optional DVNs / confirmations from inherit to pinned. Migrate the lzapp-migration example configs accordingly. [BLOCK 1] - lockfile: drop the bn.js dependency that de-linked workspace packages; the Solana u64 BN conversion now goes through a new devtools-solana bigIntToBN helper, leaving pnpm-lock.yaml byte-identical to main. [BLOCK 2] - export NIL_DVN_COUNT / NIL_CONFIRMATIONS from protocol-devtools and consume them in the EVM + Solana SDKs instead of duplicated local copies. [NIT 3] - add Solana hasAppUlnConfig idempotency unit tests. [NIT 1] - changeset: note the read-type required-field widening as a breaking change. [NIT 4] - carry requiredDVNCount/optionalDVNCount through the test-setup and example read-config constructors widened by the new required fields.
Contributor
🧪 E2E Test StatusE2E tests are non-blocking and validate real blockchain interactions. Failures may occur due to network issues, RPC rate limits, or external service downtime. Test Runs (Newest First):
|
…lana unit tests in CI - generator (creatUlnConfig): the requiredDVNs branch now emits `requiredDVNCount: 0` for the inherit case so a regenerated config re-applies as inherit instead of deriving the NIL sentinel from the empty array (matching the confirmations / optionalDVNs handling). Pinned-none still emits `[]`. [review QUESTION 1] - protocol-devtools-solana bin/test: run the pure unit suites (serializer / schema transforms, mocked reads) unconditionally; keep only the live-RPC endpointv2 SDK suite gated behind LZ_DEVTOOLS_ENABLE_SOLANA_TESTS, so the Solana logic is covered in CI. [review NIT 1]
…library path Mirror the ULN302 completion onto ulnRead: add an optional requiredDVNCount override to UlnReadUlnUserConfig/schema, honor it in serializeUlnConfig, and have the Read config generator emit requiredDVNCount: 0 for inherited required DVNs (and omit optionalDVNs for inherit). Without this, the normalizeUlnConfig swap on the chain side left the user side mapping empty requiredDVNs/optionalDVNs to NIL, flipping an inherited Read config to pinned-none on the next wire.
…nto uln302/resolve The requiredDVNCount/optionalDVNCount/confirmations NIL resolution was duplicated byte-for-byte across the EVM uln302, EVM ulnRead, and Solana uln302 serializers; only the per-chain DVN-array encoding genuinely varied. Extract resolveRequiredDVNCount, resolveOptionalDVNCount, and resolveConfirmations next to the sentinels so the three serializers stay in lock-step. Behavior-preserving.
The example configs now omit optionalDVNs to inherit the default, so the local encodeUlnConfig read config.optionalDVNs.length on undefined and threw before producing a diff. Guard the .length reads the same way the array fields below already are (|| []).
…unt resolution requiredDVNs was mandatory on the user config, so an empty array was ambiguous between inherit and pin-none and needed a requiredDVNCount override to express inherit. Make requiredDVNs optional instead, so it behaves exactly like optionalDVNs: omitted -> inherit, [] -> pin none (NIL), concrete -> pin those. This removes the requiredDVNCount override from both user configs, collapses resolveRequiredDVNCount/resolveOptionalDVNCount into a single resolveDVNCount, and simplifies both generators to emit requiredDVNs identically to optionalDVNs (omit for inherit). metadata-tools no longer sets requiredDVNCount (the serializer derives it from the array); generated configs are byte-identical after serialization.
The example configs omit optionalDVNThreshold (and could omit confirmations), but encodeUlnConfig passed them raw to uint8/uint64 ABI slots, so ethers threw INVALID_ARGUMENT on undefined. Default both scalars (?? 0), matching the .length guards already applied to the DVN arrays and the SDK serializers' own defaulting.
generateConnectionsConfig now emits an empty optional-DVN set as an explicit 'no optional DVNs' (NIL sentinel) rather than inheriting the on-chain default. Document the intended behavior so it is not a silent cascade bump.
Correct the metadata-tools changeset's inaccurate 'effective security is unchanged' claim: pinning an empty optional-DVN set CAN drop an optional quorum a pathway was inheriting from the default. That is intended — the goal is that a team's security config is exactly what their file says and cannot be changed by a LayerZero-controlled default update. State that motivation in the primary changeset too.
…ULN config requiredDVNs is now optional on the user config, but the same type backs setDefaultUlnConfig (serializeUlnConfig(config, false)). A default config with no required DVNs serializes to requiredDVNCount 0, which the contract rejects (_assertAtLeastOneDVN) — previously an always-invalid default typechecked and only failed on-chain. Throw early with a clear message on the default path instead.
creatUlnConfig/createReadUlnConfig are the count->array inverse of the serializer (inherit->omit, NIL->[], concrete->array) and had no coverage; a flipped branch would silently swap inherit<->pinned on regeneration. Add AST-print tests over the three-state matrix for both generators.
The default-config guard added in 8a43cf7 was stricter than the contract: it rejected any default without required DVNs, but _assertAtLeastOneDVN only reverts when requiredDVNCount == 0 AND optionalDVNThreshold == 0. Run the guard after resolution and check the CLAMPED threshold (a threshold with no concrete optional DVNs clamps to 0, so it is not a real quorum). An optional-only default is now allowed; an empty/threshold-only one still throws early.
…iant The length-derived DVN counts are valid only because callers feed resolved configs (getConfig -> getUlnConfig collapses NIL to 0). Uln301 inherits UlnBase, so its stored config CAN hold a NIL sentinel; pointing this at a raw config would silently drop it.
requiredDVNs is now optional on the user config; buildConfig already defaults optionalDVNs to [] but passed requiredDVNs straight to returnChecksums (expects string[]), so a config that omits requiredDVNs would crash at runtime. Default it the same way.
The previous guard defaulted an omitted requiredDVNs to [], but buildConfig's encoder maps an empty required set to NIL_DVN_COUNT (pin 'no required DVNs'), never to 0 (inherit). So omitting requiredDVNs silently wired the least-secure shape — the opposite of the changeset's 'omit to inherit'. This path cannot express inherit, so throw a clear error instead; callers must pass the DVNs explicitly, or [] to pin none. Add buildConfig tests for omitted/empty/concrete.
…gic into resolve.ts Dedup the chain-agnostic logic that had drifted into copies across the serializers and generators, and add the missing contract invariant: - resolveOptionalDVNThreshold: the threshold clamp (0 unless concrete optional DVNs), shared by all three serializers instead of three inline copies. - assertValidDefaultConfig: DEFAULT-path validation mirroring _assertAtLeastOneDVN AND the optionalDVNThreshold <= optionalDVNCount invariant (a clear local error vs an opaque revert). Scoped to the default path so an OApp config diff never throws. - dvnsFromCount: the count->array inverse (NIL->[], 0->omit, else->array) shared by both config generators instead of two hand-rolled copies. Adds default-path threshold>count + threshold-clamp tests across EVM uln302/ulnRead and Solana.
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.
What does this PR do?
Makes the devtools ULN302 serializers distinguish an explicitly-empty config field from an omitted one, matching the protocol contract's sentinel semantics:
confirmations: 0n→NIL_CONFIRMATIONS(type(uint64).max)optionalDVNs: []→NIL_DVN_COUNT(0xff), matching the existingrequiredDVNs: []0(inherit the on-chain default)To support this,
Uln302UlnConfig/UlnReadUlnConfignow carryoptionalDVNCount(andUlnReadUlnConfigalso carriesrequiredDVNCount) so a stored sentinel round-trips through the config diff. The on-chain read path no longer re-applies the empty→NIL mapping, keepinghasAppUlnConfigidempotent. The library-wide DEFAULT config still serializes literal values (it rejects NIL on-chain). On Solana,confirmationsis now encoded as aBNso theu64NIL sentinel survives without precision loss (the previousNumber(...)cast was lossy). Covers EVM (uln302 + ulnRead) and Solana.Why is this change needed?
Previously an explicitly-empty field (
confirmations: 0,optionalDVNs: []) was treated identically to an omitted one — both fell back to the protocol default. An integrator pinning "zero confirmations" or "no optional DVNs" silently got the default instead, a correctness and security gap.Type of Change
How was this tested?
protocol-devtools-evm: 70 tests pass (added confirmations/optionalDVNs matrices +
hasAppUlnConfiground-trip/idempotency tests). Solana unit tests added for the serializer NIL behavior and theSetConfigSchemaBN precision (11 pass). metadata-tools: 39 tests + 9 snapshots unchanged. All three packages build (typecheck) and lint clean. Solana mainnet-RPC integration tests are CI-disabled; the two ULN-config ones were updated to the new semantics.Impact/Screenshots
N/A
Checklist
AI Provenance