Implement fees in outbound router since it is not supported #941
Conversation
| @@ -103,16 +103,11 @@ where | |||
| })?; | |||
|
|
|||
| let mut converter = XcmConverter::new(&message, &gateway_network, &gateway_address); | |||
| let (agent_execute_command, max_target_fee) = converter.convert().map_err(|err|{ | |||
| let (agent_execute_command, _max_target_fee) = converter.convert().map_err(|err|{ | |||
There was a problem hiding this comment.
I guess _max_target_fee can be removed in its entirety here.
| if fee_asset.len() == 1 && fee_asset.contains(execution_fee) => | ||
| Some(execution_fee), | ||
| _ => return Err(BuyExecutionExpected), | ||
| }, | ||
| UnpaidExecution { check_origin: None, weight_limit: Unlimited } => None, |
There was a problem hiding this comment.
Only allow unpaid execution here since we don't handle fees on the target platform.
TBC: I am not sure this is acceptable for going live on Kusama, we might have to implement it before then.
There was a problem hiding this comment.
For implementing fee charge mechanism there are several xcm conponents related.
Firstly In source chain it requires a properly configured SovereignPaidRemoteExporter like what we did for template runtime(not the unpaid currently in assetHub).
Secondly we need to make sure from whom to charge the fee, I would assume a soverign account derivated from that particular agent_id, could be relevant to we're discussing in #940 (comment)
Lastly a properly configured FeeManager in bridgeHub which still missing.
There was a problem hiding this comment.
I am not sure if removing this code resolves the audit issue. The audit report suggests handling both scenarios. This PR makes no change to the observable behavior of this code. Both the old code and the new code will return SendError::Unroutable when target fees are specified. The only change is what error gets logged out to the log file. The new code will log TargetFeesExpected as the error. The old code will log TargetFeesNotSupported as the error, which I think is more self documenting.
So maybe this should be solved in the way Ron suggested.
There was a problem hiding this comment.
Thanks for the feedback.
@yrong I understood these fees as fees on the target chain, right? So the fees also need to be paid on the Ethereum side?
There was a problem hiding this comment.
@alistair-singh Ok sure! I thought we would just remove the fee XCM instructions and only allow UnpaidExecution for now (I know it doesn't really make a change to the functionality, but neither does some of the other refactor issues), but if you think it's better to implement the fees now I will get on it.
There was a problem hiding this comment.
In the end all incurred costs(including gas cost on the Ethereum side) should be covered by the original sender(i.e. a signed origin like Alice or Bob in assetHub).
Though we have some fee_info logic prepared to extract fee from the original xcm, we don't support it as you can see the check here actually only unpaid is allowed.
For fully implementing the fee charge mechanism it requires several other xcm components as I mentioned above which still missing(considerable change required both in assetHub and bridgeHub).
I think the current implementation should be fine for demo purposes or the launch on Rococo testnet. But definitely not for production usage as you mentioned before the kusama or polkadot.
I don't know if we would implement it right now or later, leave to the team for the final decision.
There was a problem hiding this comment.
I mostly agree with Ron. In general there are some missing pieces in BridgeHub, and we are on a very old version of Polkadot/Substrate/Cumulus/. So it is hard to make sense of where we need to be.
The big difference is that in the newer versions, AssetHub will not be configured with a SovereignRemoteExporter or a UnpaidRemoteExporter. Instead AssetHub will directly send an XCM message to BridgeHub containing an ExportMessage instruction.
So I think we should consider this PR BLOCKED for the time being.
There was a problem hiding this comment.
Seems they're just trying to reuse the previous reserve_transfer_assets without introducing a new pallet for this?
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #941 +/- ##
=======================================
Coverage 74.23% 74.23%
=======================================
Files 41 41
Lines 1836 1836
Branches 77 77
=======================================
Hits 1363 1363
Misses 451 451
Partials 22 22
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
claravanstaden
changed the title
claravanstaden
changed the title
|
Closing this PR since #940 (review) covers this. |
Remove support for target chain fees in the XCM message, since they are not handled in the target chain at all. In any case, target chain fees will be added in: #924
Resolves: SNO-645
Cumulus companion: Snowfork/cumulus#62