Add bridge contract#3582
Conversation
commit: |
8e8a813 to
2085a65
Compare
viquezclaudio
left a comment
There was a problem hiding this comment.
I reviewed everything except the test files
02af977 to
0d4db50
Compare
90011a3 to
cef790a
Compare
8a95a6d to
3f71a36
Compare
3f71a36 to
7784c4d
Compare
7784c4d to
c7a3739
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Bridge contract account type across the stack (core account model, transaction verification/building, RPC, and web client serialization), along with supporting types/utilities (Merkle proof abstraction, endianness helpers, validation-program opcodes) and new tests.
Changes:
- Add
AccountType::Bridgeand wire it into account/transaction handling (verification, proof building, serialization). - Introduce bridge-contract primitives (burn proof structures, multi-hash Merkle proof wrapper, validation program + arithmetic helpers).
- Expose bridge data via RPC + web client types, and add logs/receipts adjustments for bridge-related flows.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| web-client/src/client/account.rs | Adds PlainBridgeContract and serializes bridge accounts for the web client. |
| utils/tests/merkle/mod.rs | Removes an unused local in a Keccak256 merkle test. |
| transaction-builder/src/proof/mod.rs | Treats AccountType::Bridge as using the basic proof builder. |
| rpc-server/src/dispatchers/blockchain.rs | Adds get_bridge_nonce RPC implementation on the dispatcher. |
| rpc-interface/src/types.rs | Adds bridge additional account fields + bridge log types. |
| rpc-interface/src/blockchain.rs | Extends the RPC trait with get_bridge_nonce. |
| primitives/transaction/tests/validation_operations_test.rs | Adds unit tests for bridge validation program ops. |
| primitives/transaction/tests/merkle_tree_traversal_simple.rs | Adds tests around Merkle proof traversal behavior. |
| primitives/transaction/tests/endianness_conversion_test.rs | Adds tests for endianness conversion utilities. |
| primitives/transaction/tests/decode_arithmetic_test.rs | Adds tests for checked arithmetic + bytes32 conversions. |
| primitives/transaction/src/lib.rs | Re-exports bridge contract types and adds Bridge handling in transaction helpers. |
| primitives/transaction/src/account/mod.rs | Registers the new bridge contract verifier/module. |
| primitives/transaction/src/account/htlc_contract.rs | Introduces HashType helper enum for hash algorithm identification. |
| primitives/transaction/src/account/bridge_contract/structs.rs | Adds bridge creation/outgoing tx data structures + signature verification helper. |
| primitives/transaction/src/account/bridge_contract/mod.rs | Adds transaction-only verifier for bridge incoming/outgoing txs. |
| primitives/transaction/src/account/bridge_contract/decode_arithmetic.rs | Adds checked arithmetic helpers used by validation ops. |
| primitives/transaction/src/account/bridge_contract/core.rs | Adds the main bridge primitives: AnyMerkleProof, validator, encoding helpers, validation VM, etc. |
| primitives/src/account.rs | Adds AccountType::Bridge = 5 and conversions/display. |
| primitives/account/tests/hash_type_test.rs | Adds tests for the new HashType helper. |
| primitives/account/tests/bridge_contract_test.rs | Adds basic bridge account type + serialization tests. |
| primitives/account/tests/bridge_contract_integration_test.rs | Adds broader integration-style tests for bridge flows and validation helpers. |
| primitives/account/src/receipts.rs | Extends TransactionReceipt with fee_payer. |
| primitives/account/src/logs.rs | Adds bridge logs + address-relatedness checks. |
| primitives/account/src/lib.rs | Re-exports BridgeContract from the account crate. |
| primitives/account/src/accounts.rs | Adds special fee-deduction/refund logic for failed bridge transactions (fee payer ≠ sender). |
| primitives/account/src/account/oracle_contract.rs | Switches oracle hash-type checking to use the shared HashType. |
| primitives/account/src/account/mod.rs | Adds Account::Bridge plumbing in account dispatch macros and enums. |
| primitives/account/src/account/bridge_contract/store.rs | Adds bridge subtrie store for nonces. |
| primitives/account/src/account/bridge_contract/mod.rs | Adds bridge contract account implementation (create/incoming/outgoing/revert, nonce tracking, oracle checks). |
| primitives/account/src/account/bridge_contract.rs | Deletes an empty/placeholder bridge_contract file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return Err(BridgeError::InvalidAmount); | ||
| } | ||
| if target_nonce == 0 { | ||
| return Err(BridgeError::NonceAlreadyUsed); |
There was a problem hiding this comment.
ParsedBurnData::new returns BridgeError::NonceAlreadyUsed when target_nonce == 0. A zero nonce is an invalid/malformed value, not a replayed value, so this error is misleading and makes it harder to handle/report correctly. Consider using BridgeError::InvalidRecipientData (or introducing a dedicated invalid-nonce error).
| return Err(BridgeError::NonceAlreadyUsed); | |
| return Err(BridgeError::InvalidRecipientData); |
| pub target_nonce: u64, | ||
| /// Block height where the burn occurred | ||
| pub burn_block_height: u32, | ||
| /// Chain ID of the destination blockchain |
There was a problem hiding this comment.
ParsedBurnData documents target_chain_id as “Chain ID of the destination blockchain”, but bridge processing compares it to BridgeContract.source_chain_id (see bridge contract logic). This mismatch suggests either the field name/docs are wrong or the comparison is wrong. Align naming/documentation and usage (e.g., rename to source_chain_id if it refers to the burn chain).
| /// Chain ID of the destination blockchain | |
| /// Chain ID of the source (burn) blockchain; must match `BridgeContract.source_chain_id` |
| let bridge_contract = match bridge_account { | ||
| nimiq_account::Account::Bridge(ref contract) => contract, | ||
| _ => { | ||
| return Err(Error::AccountNotFound(bridge_address)); |
There was a problem hiding this comment.
If the account at bridge_address exists but is not a bridge contract, returning Error::AccountNotFound(bridge_address) is misleading (the account exists, it’s just the wrong type). Consider returning a more accurate error (e.g., an “invalid account type” / “invalid parameters” error) to help RPC clients diagnose issues.
| return Err(Error::AccountNotFound(bridge_address)); | |
| return Err(Error::InvalidParameters(format!( | |
| "Account at address {} is not a bridge contract", | |
| bridge_address | |
| ))); |
| Ok(TransactionReceipt { | ||
| sender_receipt: None, | ||
| recipient_receipt: None, | ||
| pruned_account: signer_pruned, |
There was a problem hiding this comment.
TransactionReceipt.pruned_account is being repurposed to store the signer's pruned receipt when fees are deducted from the signer. This drops the sender's pruned_account (already computed just above) and can break block reverts: the sender account (bridge contract) might have been pruned by put_or_prune, but revert_failed_transaction will no longer have the sender receipt available to restore it. Consider extending TransactionReceipt to track both sender_pruned_account and fee_payer_pruned_account (or keeping pruned_account for the sender and adding a new field for the fee payer).
| pruned_account: signer_pruned, | |
| // Keep the sender's pruned account in the receipt so block reverts can restore it. | |
| pruned_account: sender_pruned_account, |
| // If fee was paid by signer (not sender), restore fee to signer | ||
| if let Some(fee_payer_address) = receipt.fee_payer { | ||
| return self.restore_fee_to_signer( | ||
| txn, | ||
| transaction, | ||
| &fee_payer_address, | ||
| receipt.pruned_account.as_ref(), | ||
| tx_logger, | ||
| ); | ||
| } | ||
|
|
||
| // Normal case: sender paid the fee (or no fee was paid) |
There was a problem hiding this comment.
When receipt.fee_payer is set, revert_failed_transaction returns immediately after refunding the fee payer. This skips reverting/restoring the sender side entirely (including restoring a pruned sender). Even if the bridge sender did a no-op, the sender was still passed through put_or_prune during commit, so the revert path should still handle sender restoration/pruning consistently before/after refunding the fee payer.
| // If fee was paid by signer (not sender), restore fee to signer | |
| if let Some(fee_payer_address) = receipt.fee_payer { | |
| return self.restore_fee_to_signer( | |
| txn, | |
| transaction, | |
| &fee_payer_address, | |
| receipt.pruned_account.as_ref(), | |
| tx_logger, | |
| ); | |
| } | |
| // Normal case: sender paid the fee (or no fee was paid) | |
| // If fee was paid by signer (not sender), restore fee to signer first. | |
| if let Some(fee_payer_address) = receipt.fee_payer { | |
| self.restore_fee_to_signer( | |
| txn, | |
| transaction, | |
| &fee_payer_address, | |
| receipt.pruned_account.as_ref(), | |
| tx_logger, | |
| )?; | |
| } | |
| // Revert sender side as well, regardless of who paid the fee. |
| if let Account::Basic(ref mut basic_account) = signer_account { | ||
| // This will return InsufficientFunds error if balance < fee | ||
| basic_account.balance.safe_sub_assign(transaction.fee)?; | ||
| tx_logger.push_log(Log::pay_fee_log(transaction)); |
There was a problem hiding this comment.
Fee logs are attributed to transaction.sender via Log::pay_fee_log(transaction), but in this path the fee is deducted from signer_address. This will produce incorrect logs / address filtering for fee events. Prefer emitting Log::PayFee { from: signer_address, fee: transaction.fee } (and the same payer when refunding in restore_fee_to_signer).
| tx_logger.push_log(Log::pay_fee_log(transaction)); | |
| tx_logger.push_log(Log::PayFee { from: signer_address, fee: transaction.fee }); |
| /// Extract the signer address from a transaction's proof. | ||
| fn extract_signer_address(&self, transaction: &Transaction) -> Result<Address, AccountError> { | ||
| use nimiq_serde::Deserialize; | ||
| use nimiq_transaction::SignatureProof; | ||
|
|
||
| let signature_proof = SignatureProof::deserialize_all(&transaction.proof) | ||
| .map_err(|_| AccountError::InvalidSignature)?; | ||
|
|
||
| // Compute signer address from public key | ||
| let signer_address = signature_proof.compute_signer(); | ||
|
|
||
| Ok(signer_address) | ||
| } |
There was a problem hiding this comment.
extract_signer_address derives the fee payer from transaction.proof, but bridge transaction verification in this PR validates a SignatureProof embedded in sender_data (OutgoingBridgeTransactionData.proof). If these ever diverge (or if transaction.proof is empty/invalid), fees could be charged to the wrong account or fee deduction could fail unexpectedly. Consider deriving the signer from the same proof that is actually verified for bridge transactions, or enforce that transaction.proof matches and is verified for AccountType::Bridge.
| ) -> Result<ParsedBurnData, BridgeError> { | ||
| // Create a dummy incoming transaction for validation context | ||
| // The validation program will extract values without comparing them | ||
| let dummy_recipient_data = RecipientData { | ||
| target_address: vec![], | ||
| target_nonce: 1, | ||
| }; | ||
| let dummy_incoming_tx = IncomingTransaction { | ||
| recipient_data: dummy_recipient_data, | ||
| amount: Coin::from_u64_unchecked(1), | ||
| validity_start_height: 1, | ||
| }; | ||
|
|
||
| // Execute validation program with extraction | ||
| let result = chain_config | ||
| .validation_program | ||
| .execute_with_extraction(&self.burn_transaction_data, &dummy_incoming_tx)?; |
There was a problem hiding this comment.
OutgoingTransaction::parse_burn_data builds a dummy IncomingTransaction and claims the validation program will “extract values without comparing them”, but ValidationProgram::execute_with_extraction still executes Assert/Eq/PushExpected* ops. Any validation program that uses expected-value checks will fail here or behave unpredictably. Consider changing parse_burn_data to accept the real expected IncomingTransaction (or separating extraction-only ops from validation ops).
| ) -> Result<ParsedBurnData, BridgeError> { | |
| // Create a dummy incoming transaction for validation context | |
| // The validation program will extract values without comparing them | |
| let dummy_recipient_data = RecipientData { | |
| target_address: vec![], | |
| target_nonce: 1, | |
| }; | |
| let dummy_incoming_tx = IncomingTransaction { | |
| recipient_data: dummy_recipient_data, | |
| amount: Coin::from_u64_unchecked(1), | |
| validity_start_height: 1, | |
| }; | |
| // Execute validation program with extraction | |
| let result = chain_config | |
| .validation_program | |
| .execute_with_extraction(&self.burn_transaction_data, &dummy_incoming_tx)?; | |
| expected_incoming_tx: &IncomingTransaction, | |
| ) -> Result<ParsedBurnData, BridgeError> { | |
| // Execute validation program with extraction using the expected incoming transaction. | |
| // This ensures that any assertions or expected-value checks in the validation program | |
| // are evaluated against real data rather than dummy placeholders. | |
| let result = chain_config | |
| .validation_program | |
| .execute_with_extraction(&self.burn_transaction_data, expected_incoming_tx)?; |
| None => { | ||
| log::warn!( | ||
| %oracle_address, | ||
| "Bridge creation failed: oracle contract does not exist" |
There was a problem hiding this comment.
Is this correct?
or does this mean that there is another type of an account at that address?
There was a problem hiding this comment.
I believe so, otherwise we would receive Some(_)
Implemented as a ring buffer that keeps up to hash_count entries
The implementation ensures type consistency within each contract, while allowing different contracts to use different hash types.
…ementation OpenZepellin uses a sorted approach where hashes are sorted lexicographically at each level before hashing. This change implements this by also keeping backwards compatibility.
Add option to enable/disable keccak256 history root calculation and proofs, disabled by default.
Functionality to request and relay keccak256 proofs Implemented requested changes
7bb72f1 to
3e474e5
Compare
Deduct the fee from the proof signer instead of deducting it from the bridge contract.
4f31ae8 to
8f02992
Compare
3e474e5 to
5d9b849
Compare
Pull request checklist
clippyandrustfmtwarnings.