Repository navigation
add Argo revert_outbound_transfer extrinsic - #5158
Conversation
freakstatic
left a comment
freakstatic
left a comment
There was a problem hiding this comment.
LGTM!
@kdembler do you want me to handle the benchmark tests for this or are you gonna do that as well?
|
@freakstatic if you could do that, that'd be great |
dobertRowneySr
left a comment
dobertRowneySr
left a comment
There was a problem hiding this comment.
Logic looks ok, I have just left a few points
| #[weight = WeightInfoArgo::<T>::finalize_inbound_transfer()] | ||
| pub fn revert_outbound_transfer( | ||
| origin, | ||
| transfer_id: TransferId, |
There was a problem hiding this comment.
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.
Namely the extr. assumes that transfer_id will refer to a pending transfer outbound request queued in the argo-squid
There was a problem hiding this comment.
Correct, likewise we don't check it any way when calling finaliza_inbound_transfer. It all assumes that the operator is acting correctly
| 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.
Should this be present?
I am thinking about the following scenario:
- Transaction outbound exists
- Bridge is paused
- User tries to revert transaction
There was a problem hiding this comment.
Yes, when the bridge is paused, nothing should be callable. Also it's not user that reverts the transaction, it's the operator
| )); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Despite my previous comment I will point this out :
transfer revert failing test case for bridge status not active is missing
There was a problem hiding this comment.
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
3 participants
Based on #5155
Adds a new
revert_outbound_transferextrinsic. Functionally, the new extrinsic is almost the same asfinalize_inbound_transfer:The only difference is in input arguments and the emitted event.
Todo: