-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add Argo revert_outbound_transfer extrinsic #5158
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,6 +137,33 @@ decl_module! { | |
Ok(()) | ||
} | ||
|
||
// TODO: add weight for revert_outbound_transfer | ||
#[weight = WeightInfoArgo::<T>::finalize_inbound_transfer()] | ||
pub fn revert_outbound_transfer( | ||
origin, | ||
transfer_id: TransferId, | ||
revert_account: T::AccountId, | ||
revert_amount: BalanceOf<T>, | ||
rationale: vec::Vec<u8>, | ||
) -> DispatchResult { | ||
ensure!(Self::operator_account().is_some(), Error::<T>::OperatorAccountNotSet); | ||
let caller = ensure_signed(origin)?; | ||
ensure!(caller == Self::operator_account().unwrap(), Error::<T>::NotOperatorAccount); | ||
|
||
ensure!(Self::status() == BridgeStatus::Active, Error::<T>::BridgeNotActive); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be present?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, when the bridge is paused, nothing should be callable. Also it's not user that reverts the transaction, it's the operator |
||
ensure!(revert_amount <= Self::mint_allowance(), Error::<T>::InsufficientBridgeMintAllowance); | ||
|
||
<MintAllowance<T>>::put(Self::mint_allowance() - revert_amount); | ||
let _ = balances::Pallet::<T>::deposit_creating( | ||
&revert_account, | ||
revert_amount | ||
); | ||
|
||
Self::deposit_event(RawEvent::OutboundTransferReverted(transfer_id, revert_account, revert_amount, rationale)); | ||
|
||
Ok(()) | ||
} | ||
|
||
#[weight = WeightInfoArgo::<T>::pause_bridge()] | ||
pub fn pause_bridge(origin) -> DispatchResult { | ||
let caller = ensure_signed(origin)?; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -316,6 +316,109 @@ fn finalize_inbound_transfer_with_insufficient_bridge_mint() { | |
}); | ||
} | ||
|
||
#[test] | ||
fn revert_outbound_transfer_success() { | ||
with_test_externalities_custom_mint_allowance(joy!(1000), || { | ||
let remote_chains = BoundedVec::try_from(vec![1u32]).unwrap(); | ||
let parameters = BridgeConstraints { | ||
operator_account: Some(account!(1)), | ||
pauser_accounts: None, | ||
bridging_fee: None, | ||
thawn_duration: None, | ||
remote_chains: Some(remote_chains), | ||
}; | ||
ArgoBridge::update_bridge_constrains(RuntimeOrigin::root(), parameters).unwrap(); | ||
|
||
let transfer_id = 1u64; | ||
let revert_amount = joy!(123); | ||
let revert_account = account!(2); | ||
let rationale = "test".as_bytes().to_vec(); | ||
let result = ArgoBridge::revert_outbound_transfer( | ||
RuntimeOrigin::signed(account!(1)), | ||
transfer_id, | ||
revert_account, | ||
revert_amount, | ||
rationale.clone(), | ||
); | ||
assert_ok!(result); | ||
assert_eq!(Balances::free_balance(revert_account), revert_amount); | ||
last_event_eq!(RawEvent::OutboundTransferReverted( | ||
transfer_id, | ||
revert_account, | ||
revert_amount, | ||
rationale, | ||
)); | ||
}); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Despite my previous comment I will point this out : There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we should add this as well. Let me do this in next PR though, at this branch there are small issues with bridge status that I fixed on my next branch |
||
#[test] | ||
fn revert_outbound_transfer_with_no_operator_account() { | ||
with_test_externalities(|| { | ||
let result = ArgoBridge::revert_outbound_transfer( | ||
RuntimeOrigin::signed(account!(1)), | ||
1u64, | ||
account!(2), | ||
joy!(123), | ||
vec![], | ||
); | ||
assert_err!(result, Error::<Test>::OperatorAccountNotSet); | ||
}); | ||
} | ||
|
||
#[test] | ||
fn revert_outbound_transfer_with_unauthorized_account() { | ||
with_test_externalities(|| { | ||
let remote_chains = BoundedVec::try_from(vec![1u32]).unwrap(); | ||
let parameters = BridgeConstraints { | ||
operator_account: Some(account!(1)), | ||
pauser_accounts: None, | ||
bridging_fee: None, | ||
thawn_duration: None, | ||
remote_chains: Some(remote_chains), | ||
}; | ||
assert_ok!(ArgoBridge::update_bridge_constrains( | ||
RuntimeOrigin::root(), | ||
parameters | ||
)); | ||
|
||
let result = ArgoBridge::revert_outbound_transfer( | ||
RuntimeOrigin::signed(account!(2)), | ||
1u64, | ||
account!(2), | ||
joy!(123), | ||
vec![], | ||
); | ||
assert_err!(result, Error::<Test>::NotOperatorAccount); | ||
}); | ||
} | ||
|
||
#[test] | ||
fn revert_outbound_transfer_with_insufficient_bridge_mint() { | ||
with_test_externalities(|| { | ||
let remote_chains = BoundedVec::try_from(vec![1u32]).unwrap(); | ||
let parameters = BridgeConstraints { | ||
operator_account: Some(account!(1)), | ||
pauser_accounts: None, | ||
bridging_fee: None, | ||
thawn_duration: None, | ||
remote_chains: Some(remote_chains), | ||
}; | ||
assert_ok!(ArgoBridge::update_bridge_constrains( | ||
RuntimeOrigin::root(), | ||
parameters | ||
)); | ||
|
||
let result = ArgoBridge::revert_outbound_transfer( | ||
RuntimeOrigin::signed(account!(1)), | ||
1u64, | ||
account!(2), | ||
joy!(100), | ||
vec![], | ||
); | ||
assert_err!(result, Error::<Test>::InsufficientBridgeMintAllowance); | ||
}); | ||
} | ||
|
||
#[test] | ||
fn pause_bridge_success() { | ||
with_test_externalities(|| { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that this is an identifier whose validity is already confirmed by the squid.
Given that there are no checks on this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Namely the extr. assumes that
transfer_id
will refer to a pending transfer outbound request queued in the argo-squidThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, likewise we don't check it any way when calling
finaliza_inbound_transfer
. It all assumes that the operator is acting correctly