From 196d10c5ae7d18055562d1f075f3ad5c277920dc Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 14 Sep 2023 13:18:12 +0100 Subject: [PATCH] Refactor validate_changed_keys in the Bridge pool VP --- .../ethereum_bridge/bridge_pool_vp.rs | 155 ++++++------------ 1 file changed, 54 insertions(+), 101 deletions(-) diff --git a/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs b/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs index 22fee8b2f0a..9894c6e85ad 100644 --- a/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs +++ b/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs @@ -387,6 +387,7 @@ where escrow_account: &BRIDGE_POOL_ADDRESS, expected_debit: expected_gas_debit, expected_credit: expected_gas_credit, + transferred_amount: &transfer.gas_fee.amount, _kind: PhantomData, }, token_check: EscrowDelta { @@ -395,6 +396,7 @@ where escrow_account: token_check_escrow_acc, expected_debit: expected_token_debit, expected_credit: expected_token_credit, + transferred_amount: &transfer.transfer.amount, _kind: PhantomData, }, }) @@ -409,15 +411,31 @@ struct EscrowDelta<'a, KIND> { escrow_account: &'a Address, expected_debit: Amount, expected_credit: Amount, + transferred_amount: &'a Amount, _kind: PhantomData<*const KIND>, } -impl EscrowDelta<'_, TokenCheck> { - fn validate_changed_keys( - &self, - changed_keys: &BTreeSet, - pending: &PendingTransfer, - ) -> bool { +impl EscrowDelta<'_, KIND> { + /// Validate an [`EscrowDelta`]. + /// + /// # Conditions for validation + /// + /// If the transferred amount in ihe [`EscrowDelta`] is nil, + /// then no keys could have been changed. If the transferred + /// amount is greater than zero, then the appropriate escrow + /// keys must have been written to by some wasm tx. + #[inline] + fn validate(&self, changed_keys: &BTreeSet) -> bool { + if hints::unlikely(self.transferred_amount_is_nil()) { + self.check_escrow_keys_unchanged(changed_keys) + } else { + self.check_escrow_keys_changed(changed_keys) + } + } + + /// Check if all required escrow keys in `changed_keys` were modified. + #[inline] + fn check_escrow_keys_changed(&self, changed_keys: &BTreeSet) -> bool { let EscrowDelta { token, payer_account, @@ -428,23 +446,14 @@ impl EscrowDelta<'_, TokenCheck> { let owner_key = balance_key(token, payer_account); let escrow_key = balance_key(token, escrow_account); - let has_owner_key = changed_keys.contains(&owner_key); - let has_escrow_key = changed_keys.contains(&escrow_key); - - let amount_is_zero = pending.transfer.amount.is_zero(); - - hints::likely(!amount_is_zero && has_owner_key && has_escrow_key) - || hints::unlikely( - amount_is_zero && !has_owner_key && !has_escrow_key, - ) + changed_keys.contains(&owner_key) && changed_keys.contains(&escrow_key) } -} -impl EscrowDelta<'_, GasCheck> { - fn validate_changed_keys( + /// Check if no escrow keys in `changed_keys` were modified. + #[inline] + fn check_escrow_keys_unchanged( &self, changed_keys: &BTreeSet, - pending: &PendingTransfer, ) -> bool { let EscrowDelta { token, @@ -456,15 +465,17 @@ impl EscrowDelta<'_, GasCheck> { let owner_key = balance_key(token, payer_account); let escrow_key = balance_key(token, escrow_account); - let has_owner_key = changed_keys.contains(&owner_key); - let has_escrow_key = changed_keys.contains(&escrow_key); - - let amount_is_zero = pending.gas_fee.amount.is_zero(); + !changed_keys.contains(&owner_key) + && !changed_keys.contains(&escrow_key) + } - hints::likely(!amount_is_zero && has_owner_key && has_escrow_key) - || hints::unlikely( - amount_is_zero && !has_owner_key && !has_escrow_key, - ) + /// Check if the amount transferred to escrow is nil. + #[inline] + fn transferred_amount_is_nil(&self) -> bool { + let EscrowDelta { + transferred_amount, .. + } = self; + transferred_amount.is_zero() } } @@ -479,15 +490,9 @@ struct EscrowCheck<'a> { impl EscrowCheck<'_> { #[inline] - fn validate_changed_keys( - &self, - changed_keys: &BTreeSet, - pending: &PendingTransfer, - ) -> bool { - self.gas_check.validate_changed_keys(changed_keys, pending) - && self - .token_check - .validate_changed_keys(changed_keys, pending) + fn validate(&self, changed_keys: &BTreeSet) -> bool { + self.gas_check.validate(changed_keys) + && self.token_check.validate(changed_keys) } } @@ -589,7 +594,7 @@ where let wnam_address = read_native_erc20_address(&self.ctx.pre())?; let escrow_checks = self.determine_escrow_checks(&wnam_address, &transfer)?; - if !escrow_checks.validate_changed_keys(keys_changed, &transfer) { + if !escrow_checks.validate(keys_changed) { tracing::debug!( ?transfer, "Missing storage modifications in the Bridge pool" @@ -1903,34 +1908,21 @@ mod test_bridge_pool_vp { #[test] fn test_no_gas_fees_with_no_changed_keys() { let nam_addr = nam(); - let pending = PendingTransfer { - transfer: TransferToEthereum { - kind: TransferToEthereumKind::Erc20, - asset: wnam(), - sender: bertha_address(), - recipient: EthAddress([1; 20]), - amount: Amount::from(1u64), - }, - gas_fee: GasFee { - token: nam(), - // NOTE: testing 0 amount - amount: Amount::zero(), - payer: bertha_address(), - }, - }; let delta = EscrowDelta { token: Cow::Borrowed(&nam_addr), payer_account: &bertha_address(), escrow_account: &BRIDGE_ADDRESS, expected_debit: Amount::zero(), expected_credit: Amount::zero(), + // NOTE: testing 0 amount + transferred_amount: &Amount::zero(), // NOTE: testing gas fees _kind: PhantomData::<*const GasCheck>, }; // NOTE: testing no changed keys let empty_keys = BTreeSet::new(); - assert!(delta.validate_changed_keys(&empty_keys, &pending)); + assert!(delta.validate(&empty_keys)); } /// Test that the Bridge pool native VP rejects transfers that @@ -1938,27 +1930,14 @@ mod test_bridge_pool_vp { #[test] fn test_no_gas_fees_with_changed_keys() { let nam_addr = nam(); - let pending = PendingTransfer { - transfer: TransferToEthereum { - kind: TransferToEthereumKind::Erc20, - asset: wnam(), - sender: bertha_address(), - recipient: EthAddress([1; 20]), - amount: Amount::from(1u64), - }, - gas_fee: GasFee { - token: nam(), - // NOTE: testing 0 amount - amount: Amount::zero(), - payer: bertha_address(), - }, - }; let delta = EscrowDelta { token: Cow::Borrowed(&nam_addr), payer_account: &bertha_address(), escrow_account: &BRIDGE_ADDRESS, expected_debit: Amount::zero(), expected_credit: Amount::zero(), + // NOTE: testing 0 amount + transferred_amount: &Amount::zero(), // NOTE: testing gas fees _kind: PhantomData::<*const GasCheck>, }; @@ -1966,7 +1945,7 @@ mod test_bridge_pool_vp { // NOTE: testing changed keys let some_changed_keys = BTreeSet::from([owner_key]); - assert!(!delta.validate_changed_keys(&some_changed_keys, &pending)); + assert!(!delta.validate(&some_changed_keys)); } /// Test that the Bridge pool native VP validates transfers @@ -1974,34 +1953,21 @@ mod test_bridge_pool_vp { #[test] fn test_no_amount_with_no_changed_keys() { let nam_addr = nam(); - let pending = PendingTransfer { - transfer: TransferToEthereum { - kind: TransferToEthereumKind::Erc20, - asset: wnam(), - sender: bertha_address(), - recipient: EthAddress([1; 20]), - // NOTE: testing 0 amount - amount: Amount::zero(), - }, - gas_fee: GasFee { - token: nam(), - amount: Amount::from(1u64), - payer: bertha_address(), - }, - }; let delta = EscrowDelta { token: Cow::Borrowed(&nam_addr), payer_account: &bertha_address(), escrow_account: &BRIDGE_ADDRESS, expected_debit: Amount::zero(), expected_credit: Amount::zero(), + // NOTE: testing 0 amount + transferred_amount: &Amount::zero(), // NOTE: testing token transfers _kind: PhantomData::<*const TokenCheck>, }; // NOTE: testing no changed keys let empty_keys = BTreeSet::new(); - assert!(delta.validate_changed_keys(&empty_keys, &pending)); + assert!(delta.validate(&empty_keys)); } /// Test that the Bridge pool native VP rejects transfers @@ -2009,27 +1975,14 @@ mod test_bridge_pool_vp { #[test] fn test_no_amount_with_changed_keys() { let nam_addr = nam(); - let pending = PendingTransfer { - transfer: TransferToEthereum { - kind: TransferToEthereumKind::Erc20, - asset: wnam(), - sender: bertha_address(), - recipient: EthAddress([1; 20]), - // NOTE: testing 0 amount - amount: Amount::zero(), - }, - gas_fee: GasFee { - token: nam(), - amount: Amount::from(1u64), - payer: bertha_address(), - }, - }; let delta = EscrowDelta { token: Cow::Borrowed(&nam_addr), payer_account: &bertha_address(), escrow_account: &BRIDGE_ADDRESS, expected_debit: Amount::zero(), expected_credit: Amount::zero(), + // NOTE: testing 0 amount + transferred_amount: &Amount::zero(), // NOTE: testing token transfers _kind: PhantomData::<*const TokenCheck>, }; @@ -2037,6 +1990,6 @@ mod test_bridge_pool_vp { // NOTE: testing changed keys let some_changed_keys = BTreeSet::from([owner_key]); - assert!(!delta.validate_changed_keys(&some_changed_keys, &pending)); + assert!(!delta.validate(&some_changed_keys)); } }