Fix bridge reserve transfer condition#1513
Conversation
8f1d0a9 to
502eafa
Compare
acd1328 to
334544c
Compare
c4b3303 to
be86bcf
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e6c3f0cd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bad7a4fef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ae592d9af
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
m2ux
left a comment
There was a problem hiding this comment.
PR Review Summary
PR: #1513 — Fix bridge reserve transfer condition
Reviewers: Code Review Agent · Test Suite Review Agent · Validation Agent · Strategic Review Agent
Reports: Code Review · Test Suite Review · Structural Findings · Strategic Review
Date: 2026-05-21
Tip reviewed: 0bad7a4f ("Revert to BoundedVec") on fix-bridge-reserve-transfer-condition (worktree HEAD 6e6c3f0c).
Executive Summary
The fix correctly relocates the Reserve/User discriminator from attacker-controlled metadata onto an inputs-derived aggregate that the on-chain reserve script authenticates. The classifier's core arithmetic, the SQL rewrite, the dual-emission Vec return shape, and the upgrade-safe Inert fallback are all sound. The 0bad7a4f revert to BoundedVec (with whole-transaction drop in txs_to_transfers) cleanly closes the maintainer + Codex soft-cap concerns that targeted the earlier 6e6c3f0c head. No structural or correctness blockers remain.
Two action items rise to the "Should Address" tier: the dual-emission (1,1) branch — the most-novel code path in the PR — has no test fixture, and the DR-2 on-chain dependency that the off-chain classifier silently relies on is captured only as an inline note rather than a top-of-function precondition.
Overall Rating: Comment Only with Action Items.
Open Questions for the Author
These came out of the assumptions review and are surfaced here rather than as table findings:
- (PI-02 — stakeholder) The "saturate-to-zero on ICS-net-negative" choice in
tx_to_transfers(wherebridge_in > bridge_outquietly suppresses the User dimension under DR-2) is internally consistent with #1479's text, but the issue does not pin the arithmetic. Could you confirm this is the intended behaviour, vs. an explicitRefund/ signed-amount event? (Either answer is fine; we just want it on record before the doc-comment in CR-9 names it.) - (PI-04 — architectural / out-of-PR)
StorageValue<MainChainScriptsConfiguration, OptionQuery>returnsNoneonly for missing keys; if an environment ever sets scripts under one shape and the shape later changes, the storage decoder itself panics (theunwrap_or(None)innew_v1only protects the runtime-API layer, not the underlying storage). The change file states no environment has set scripts yet, so this is latent today. Is there appetite for a follow-up issue capturing the long-term mitigation (explicit storage migration on next shape change, or a manually-decoded bytes wrapper), or do you want to defer until the first environment is configured?
Code Review Findings
| # | Finding | Severity |
|---|---|---|
| CR-4 | (reserve_debit > 0, locked_amount > 0) dual-emission branch has no test fixture; complex_transfer is named for the dual-source case but actually exercises (1,0) |
Medium |
| CR-9 | Classifier correctness is load-bearing on the on-chain reserve script restricting outputs to ICS (DR-2); the inline "Assumption #1" comment names this but it should be promoted to a top-of-function doc-comment precondition | Medium |
| CR-5 | No regression guard against the pre-PR Some(Array([])) ⇒ Reserve interpretation |
Low |
| CR-6 | Silent saturate of User events on bridge_in > bridge_out (DR-2 out-of-model) leaves no operator breadcrumb |
Low (pedagogy) |
| CR-7 | A single "Pallet not configured" log conflates three failure modes at new_v1 — most notably collapsing the SCALE-decode-failure (upgrade-path) case into the generic governance-not-set case |
Low (production-readiness) |
| CR-8 | Comment block above bridge_inputs_subquery names the ICS address only; the symmetric reserve_inputs_subquery has no analogous comment and the pair is not factored |
Nit |
CR-1, CR-2, CR-3 (raised by the Klapeyron 05-21 round + Codex bot P1s on
6e6c3f0c— soft-cap benchmark range, staleMaxTransfersPerBlockdocs, vestigialtry_into().expect) all close cleanly under0bad7a4f. They are not surfaced here; they are recorded in the report for traceability only.
Finding Details
CR-4 — Dual-emission (1,1) branch lacks a fixture (Medium)
The branch where a single Cardano tx produces both a Reserve transfer and an Address transfer is the single most-novel behaviour in tx_to_transfers (and the entire reason the return type is Vec<BridgeTransferV1> rather than Option<BridgeTransferV1>). It is statically reachable but no test exercises it. complex_transfer / complex_withdraw_tx is named to suggest the dual-source case but the SQL shape (reserve_in=150, reserve_out=100, bridge_in=2000, bridge_out=1995) saturates ics_credit to 0 and lands in (1,0) — reserve-only with the user dimension silently suppressed.
Suggestion: Add a SQL fixture with reserve_in=100, reserve_out=0, bridge_in=0, bridge_out=150, c2m_metadata=Some(Array([Bytes(0x<32-byte address>)])) and assert the return is [Reserve { amount: 100 }, Address { amount: 50, recipient }] in order. See TR-1 for the test-side framing.
CR-9 — DR-2 dependency not surfaced as a top-of-function precondition (Medium)
The classifier's correctness — specifically, that reserve_debit ≤ ics_credit for valid txs — is load-bearing on the on-chain reserve script forbidding reserve outputs that do not land at ICS. Reserve-drain-to-non-ICS and reserve-burn shapes (cases C9 / C10 in the comprehension's case analysis) are silently missed if this on-chain invariant is violated — they don't appear at all, because relevant_txs filters to txs with a bridge_outputs row. The inline // Assumption #1 - Reserve can unlock only to ICS comment names the dependency but treats it as a local note; a future contributor reading just the function signature has no signal that correctness is shared with the on-chain script.
Suggestion: Promote Assumption #1 to a doc-comment on tx_to_transfers, naming DR-2 by label and stating where the invariant is enforced (the reserve validator script, off-chain-unreadable). Concrete draft in code-review.md under CR-9.
CR-5 — Empty-array metadata regression guard (Low)
Pre-PR, metadata_to_recipient(Some(Array([]))) returned Reserve (the empty-array sentinel). Post-PR the strict [JsonValue::String(str)] pattern excludes it, mapping to Invalid. The current behaviour is correct, but a future parser refactor that adds back a smell-test branch for empty arrays could silently re-open the pre-PR attack surface without detection.
Suggestion: Add a SQL fixture with c2m_metadata = '\x80' (CBOR empty array) plus a positive ICS output and assert the returned transfer is TransferRecipient::Invalid. See TR-2.
CR-6 — Silent saturate of user events lacks an operator breadcrumb (Low)
When a Cardano tx has bridge_in > bridge_out (an ICS withdrawal shape), ics_credit saturates to 0, locked_amount saturates to 0, and no Address event is emitted. The current saturate-to-zero policy is the right design under DR-2 and the u64 event-amount type, but the classifier emits no log when this suppression happens — an operator investigating "where did this user's deposit go?" has no breadcrumb.
Suggestion: Add a log::warn! (or log::debug!) at the suppression site naming the tx hash, the input aggregates, and the DR-2 framing. See TR-5 for the optional test-side observability.
CR-7 — Inert log conflates three failure modes (Low — production-readiness)
new_v1 emits the single line "Pallet not configured." for three distinct conditions: (1) get_last_data_checkpoint(...) ⇒ Ok(None) (storage uninitialised), (2) get_main_chain_scripts(...) ⇒ Ok(None) (governance not configured), (3) get_main_chain_scripts(...) ⇒ Err(...) collapsed by .unwrap_or(None) (SCALE-decode failure — the exact upgrade-path failure the new reserve_validator_address field was designed to handle gracefully). The third case is the operator's diagnostic story for "I upgraded my node but the bridge silently stopped working" — it should be at warn! level with its own message. The first two are info!-level "expected, transient" cases.
Suggestion: Split into three distinguishable arms (concrete pattern in the code-review.md CR-7 entry). Add a unit test covering the SCALE-decode-Err-collapsed-to-None path (TR-3).
CR-8 — Comment above bridge_inputs_subquery not factored over the symmetric reserve sibling (Nit)
The bridge_inputs_subquery carries a comment explaining its purpose; the structurally identical reserve_inputs_subquery (same shape, $2 parameter instead of $1) has none, and the comment block above the first refers only to "the ICS address". Future maintainers modifying both could easily desync.
Suggestion: Factor into a helper build_inputs_subquery(address_param, tx_in_configuration), or duplicate the comment block above reserve_inputs_subquery with "the reserve address" substituted. Option (a) is preferable.
Test Review Findings
| # | Gap | Severity |
|---|---|---|
| TR-1 | Dual-emission (1,1) fixture missing (anchors CR-4) |
Medium |
| TR-3 | Inert-on-SCALE-decode-failure path is not directly tested; the upgrade-path narrative depends on this exact line surviving future refactors |
Medium |
| TR-9 | max_transfers soft-cap behaviour under the new drop-whole-tx contract is not exercised; the existing max_transfers=1 test predates the dual-emission branch |
Medium |
| TR-2 | No regression fixture for c2m_metadata = Some(Array([])) ⇒ Invalid (anchors CR-5) |
Low |
| TR-4 | Self-spend-at-ICS (C6 shape: input + output at the same ICS address, net-positive) has no dedicated fixture | Low |
| TR-5 | Saturated User-event suppression (C5 shape) is covered for outcome but not for observability (tied to CR-6) | Low |
| TR-6 | metadata_to_recipient reject paths (missing 0x prefix, odd-length hex, non-hex chars, multi-element array, non-array root, non-string element) lack focused unit tests |
Low |
| TR-7 | No property test for the conservation invariant sum(transfer.amount) == reserve_debit + locked_amount |
Low |
| TR-8 | Multi-asset bundle filter (only cNIGHT counted via policy_id + asset_name) has no dedicated fixture |
Low |
Finding Details
TR-1 — Dual-emission (1,1) fixture missing (Medium)
The dual-emission branch of tx_to_transfers —
if reserve_debit > 0 {
transfers.push(...); // Reserve
}
if locked_amount > 0 {
transfers.push(...); // Address
}— is statically reachable but exercised by no passing test. Without a fixture, a future refactor that accidentally collapses the two ifs to if/else would pass the existing suite. Under 0bad7a4f, the fixture should also exercise the "would overflow under max_transfers → drop whole tx" interaction with txs_to_transfers.
Suggestion: Add dual_emission_transfer_reserve() / dual_emission_transfer_user() fixtures and a passing-case assertion plus an overflow-drop case.
TR-3 — Inert SCALE-decode-failure path not directly tested (Medium)
The .unwrap_or(None) collapse from Err(...) on get_main_chain_scripts(...) is the exact line the change file's upgrade-path narrative depends on. A future change that replaces it with ? (propagating the error) would silently break upgrade-path safety, and no test would fail.
Suggestion: Add a unit test mocking the runtime API to return Err(...) from get_main_chain_scripts, construct the IDP via new_v1, and assert Ok(Self::Inert). A complementary test for the Ok(None) branch would protect against accidental conflation under the CR-7 log split.
TR-9 — max_transfers soft-cap under dual-emission not tested (Medium)
truncates_output_and_returns_utxo_checkpoint_if_max_output_is_reached sets max_transfers=1 and asserts a single reserve_transfer — that test predates the (1,1) branch and doesn't show what happens when a single source tx would emit 2 transfers with max_transfers=1. Under 0bad7a4f, the correct answer is "the whole tx is dropped"; this is the regression-test partner for the soft-cap fix and protects txs_to_transfers's new drop-whole-tx guarantee.
Suggestion: Add a test that combines the (1,1) fixture from TR-1 with a separate (0,1) tx, runs with max_transfers=1, and asserts the result is the user-only transfer (the dual-emission tx is dropped).
TR-2 — Some(Array([])) regression fixture missing (Low)
Tied to CR-5. Add a row with c2m_metadata = '\x80', a positive ICS output, no reserve inputs, and assert TransferRecipient::Invalid.
TR-4 — C6 self-spend-at-ICS not covered (Low)
A tx whose inputs include an ICS UTxO and whose outputs include an ICS UTxO net-positive — bridge_in > 0, bridge_out > 0, ics_credit = bridge_out - bridge_in > 0 — is statically well-defined but exercised by no fixture. Belt-and-braces against future SQL-rewrite regressions.
Suggestion: Add a fixture with bridge_in=50, bridge_out=150, no reserve, valid user metadata, assert one Address event of amount 100.
TR-5 — Observability of suppressed user events (Low)
Tied to CR-6. If CR-6's log::warn! is accepted, assert the warn fires on the C5-shaped fixture.
TR-6 — metadata_to_recipient reject paths (Low)
Six distinct reject paths; two are covered end-to-end via invalid_transfer_* fixtures. The remaining four are reachable only via direct unit tests of metadata_to_recipient. The function is pub(crate) so the test is straightforward.
TR-7 — Property test for conservation invariant (Low)
Add a proptest! over four u64 aggregates respecting DR-2 (reserve_in - reserve_out ≤ bridge_out - bridge_in) and assert sum(transfer.amount) == reserve_debit + locked_amount. Belt-and-braces; the property is the meaningful definition of correctness — case fixtures only sample it.
TR-8 — Multi-asset bundle filter (Low)
A fixture row producing one cNIGHT output at ICS and one different multi-asset output at the same address would protect the SQL policy_id + asset_name join condition against drop.
Branch Hygiene
| # | Item | Severity |
|---|---|---|
| 1 | Commit signatures over 9b9ad26c..0bad7a4f |
Pass (all %G? = E, none N/B) |
| 2 | Branch freshness vs main |
Pass (two recent merges from main; behind by 0 at composition) |
| 3 | Change note present in changes/changed/ |
Pass — documents defect, fix, dual-emission case, upgrade-path consequence; links issue + PR |
No findings.
Action Items
Should Address (Recommended):
- Add a
(1,1)dual-emission SQL fixture and corresponding test (one Reserve event + one Address event from a single source tx). Optionally extend with the "would-overflow → drop" interaction undermax_transfers. (CR-4 / TR-1 / TR-9) - Promote the inline
Assumption #1 - Reserve can unlock only to ICScomment intx_to_transfersto a top-of-function doc-comment, naming DR-2 and the on-chain enforcement site. (CR-9) - Add a unit test for the
Inert-on-SCALE-decode-failure path innew_v1(mockget_main_chain_scriptsto returnErr(...), assertOk(Self::Inert)). (TR-3)
Could Address (Suggested):
- Split the
new_v1"Pallet not configured" log into three distinguishable arms (storage-uninitialised / governance-not-set / SCALE-decode-failure) so the upgrade-path failure mode is operator-visible. (CR-7) - Add a regression fixture asserting
c2m_metadata = Some(Array([]))⇒TransferRecipient::Invalid, notReserve. (CR-5 / TR-2) - Add a
log::warn!/log::debug!at the saturated-user-event suppression site (bridge_in > bridge_out) so operators have a breadcrumb for the DR-2 out-of-model shape. (CR-6 / TR-5) - Add a fixture covering self-spend at ICS (C6 shape). (TR-4)
Nice to Have (Optional):
- Factor the symmetric
bridge_inputs_subquery/reserve_inputs_subquerypair into a helper, or duplicate the comment block with "the reserve address" substituted. (CR-8) - Add focused unit tests for the six
metadata_to_recipientreject paths. (TR-6) - Add a
proptest!for the conservation invariantsum(transfer.amount) == reserve_debit + locked_amount. (TR-7) - Add a fixture exercising the multi-asset bundle filter. (TR-8)
|
Author answers: PI-04 - intention as well. We don't provide migration path, because this is not enabled yet. Bridge is off on all environments and the way of turning it on is using the call to set addresses and initial data checkpoint. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 295a74081d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
295a740 to
62447ec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62447ec31e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cbf7b7600
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b03a289d48
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4cd7495b5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
a1ab1e5 to
127562b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 127562b6f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
127562b to
d9f6411
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9f6411ad5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
d9f6411 to
09c499f
Compare
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f623d9dccf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Overview
Fixes bug that causes wrong classification of some bridge transfers.
In order to properly classify transfer: User vs Reserve we ought to look at origin of ICS tokens surplus.
Runtime has to know reserve validator address to be able to pass it to observability. Because bridge is not deployed yet, we don't add migration here. Addresses will have to be overwritten, so they can be read with the new format.
SQL query returns tokens in and out for reserve validator and ICS validator.
Assumption for Reserve is that it allows only to transfer out to ICS. So, negative delta of Reserve address is "Reserve Transfer". Then positive delta at ICS is what user has added. This is corner case. We expect that there won't be such combined transfer, but the code is ready for such calculation.
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
Please describe any additional testing aside from CI:
🔱 Fork Strategy
Links
Fixes: #1479