Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add Argo revert_outbound_transfer extrinsic #5158

Merged
merged 1 commit into from
Jun 4, 2024
+132 −0
Merged

add Argo revert_outbound_transfer extrinsic #5158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions runtime-modules/argo-bridge/src/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use frame_support::decl_event;

use crate::{RemoteAccount, RemoteTransfer, TransferId};
use sp_std::vec::Vec;

use crate::types::*;

Expand All @@ -19,6 +20,7 @@ decl_event!(
{
OutboundTransferRequested(TransferId, AccountId, RemoteAccount, Balance, Balance),
InboundTransferFinalized(RemoteTransfer, AccountId, Balance),
OutboundTransferReverted(TransferId, AccountId, Balance, Vec<u8>),
BridgePaused(AccountId),
BridgeThawnStarted(AccountId, BlockNumber),
BridgeThawnFinished(),
Expand Down
27 changes: 27 additions & 0 deletions runtime-modules/argo-bridge/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

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...

Copy link
Contributor

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-squid

Copy link
Member Author

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be present?
I am thinking about the following scenario:

  1. Transaction outbound exists
  2. Bridge is paused
  3. User tries to revert transaction

Copy link
Member Author

Choose a reason for hiding this comment

The 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)?;
Expand Down
103 changes: 103 additions & 0 deletions runtime-modules/argo-bridge/src/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
));
});
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite my previous comment I will point this out :
transfer revert failing test case for bridge status not active is missing

Copy link
Member Author

Choose a reason for hiding this comment

The 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(|| {
Expand Down
Loading