Skip to content

Commit

Permalink
Refactor validate_changed_keys in the Bridge pool VP
Browse files Browse the repository at this point in the history
  • Loading branch information
@sug0
sug0 committed Sep 14, 2023
1 parent ecb0009 commit 196d10c
Showing 1 changed file with 54 additions and 101 deletions.
155 changes: 54 additions & 101 deletions shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
},
})
Expand All @@ -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<Key>,
pending: &PendingTransfer,
) -> bool {
impl<KIND> 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<Key>) -> 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<Key>) -> bool {
let EscrowDelta {
token,
payer_account,
Expand All @@ -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<Key>,
pending: &PendingTransfer,
) -> bool {
let EscrowDelta {
token,
Expand All @@ -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()
}
}

Expand All @@ -479,15 +490,9 @@ struct EscrowCheck<'a> {

impl EscrowCheck<'_> {
#[inline]
fn validate_changed_keys(
&self,
changed_keys: &BTreeSet<Key>,
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<Key>) -> bool {
self.gas_check.validate(changed_keys)
&& self.token_check.validate(changed_keys)
}
}

Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -1903,140 +1908,88 @@ 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
/// do not contain gas fees and has associated changed keys.
#[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>,
};
let owner_key = balance_key(&nam_addr, &bertha_address());
// 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
/// moving no value and with no associated changed keys.
#[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
/// moving no value and with associated changed keys.
#[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>,
};
let owner_key = balance_key(&nam_addr, &bertha_address());
// 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));
}
}

0 comments on commit 196d10c

Please sign in to comment.