SNO-624: Charge fee for outbound operations #940
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #940 +/- ##
==========================================
- Coverage 75.23% 74.46% -0.77%
==========================================
Files 41 42 +1
Lines 1813 1978 +165
Branches 75 75
==========================================
+ Hits 1364 1473 +109
- Misses 429 486 +57
+ Partials 20 19 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
yrong
changed the base branch from
ron/oak-security-audit-pick-sno-584
to
main
yrong
changed the base branch from
main
to
ron/oak-security-audit-pick-sno-584
yrong
changed the base branch from
ron/oak-security-audit-pick-sno-584
to
main
yrong
changed the title
yrong
changed the title
yrong
changed the title
yrong
changed the title
There was a problem hiding this comment.
A mammoth task, this one! 😄 🐘 I've added a few comments and questions. I think overall more unit tests would be useful to test the lower level methods like charge_fees and set_outbound_fee_config.
Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
parachain/pallets/control/src/lib.rs
Outdated
| T::OutboundQueue::submit(ticket).map_err(|_| Error::<T>::SubmissionFailed)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn convert_location( | ||
| mut location: MultiLocation, | ||
| ) -> Result<(H256, Option<ParaId>, MultiLocation), DispatchError> { | ||
| ) -> Result<(H256, ParaId, MultiLocation), DispatchError> { |
There was a problem hiding this comment.
Lets rather return a struct to make the code self-documenting:
| ) -> Result<(H256, ParaId, MultiLocation), DispatchError> { | |
| ) -> Result<OriginInfo, DispatchError> { |
struct OriginInfo {
agent_id: H256,
para_id: ParaId,
location: MultiLocation,
}
| /// gas price in Wei from https://etherscan.io/gastracker | ||
| pub gas_price: Option<GasPriceInWei>, | ||
| /// swap ratio for Ether->DOT from https://www.coingecko.com/en/coins/polkadot/eth with precision difference between Ether->DOT(18->10) | ||
| pub swap_ratio: Option<FixedU128>, |
There was a problem hiding this comment.
I think it's simpler to not use Option. I.e when a new OutboundFeeConfig is updated, it completely replaces the old stored struct.
There was a problem hiding this comment.
The reason for the Option is in some cases we may want to configure it partially(e.g. only command_gas_map with the gas cost for some commands), In this case though it's possible to read previous value from storage and feed it into the call but is error prone IMO.
| fn estimate_fee(message: &Message) -> Result<MultiAssets, SubmitError>; | ||
|
|
||
| fn estimate_fee_by_command_index(command_index: u8) -> Result<MultiAssets, SubmitError>; |
There was a problem hiding this comment.
I think these APIs need some more work
For example, the APIs also need to return the reward paid out on the Ethereum side. Origin parachains need this info so that they can make sure their sovereign accounts on Ethereum are topped up.
To cater for this, the APIs could return a tuple:
- Fee charged, if any, on BridgeHub (like for CreateAgent), in DOT
- Reward paid out on Ethereum, in ETH.
Do we really need both estimate_fee and estimate_fee_by_command_index? Let's try to settle on having one good API that satisfies all use cases.
One idea:
fn compute_fee_reward(Command: &command) -> Result<(u128, u128), SubmitError> There was a problem hiding this comment.
Some revamp accordingly in a6649d8 for the reward.
Yes, though currently only command is required for computing fees. I'm afraid there is some use case that depends on ParaId(e.g. discounts for a particular parachain). So would prefer the previous Message which includes the original ParaId.
Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
|
Hey @yrong! I tried checking out your branch together with https://github.com/Snowfork/cumulus/tree/ron/sno-624 but I couldn't get it to build - I came across some compiler errors in the bridge hub rococo runtime xcm config due to the renaming of |
Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
@claravanstaden Thanks for the review! Actually the cumulus branch is |
|
Closed in favor of #968 |
Fix for audit issue (1).
Resolves: SNO-624
Requires: Snowfork/cumulus#63
Context
It makes sense to let third-party Parachain who would like to use the bridge know what the fee is in advance, so added the Estimate API into the runtime. The extra fee is related to
So we've made all these factors configurable through extrinsic. Basically for most of the commands gas cost should be static, the only exception is maybe the arbitrary transact we'll add later, the dispatch gas should be some dynamic input from third party. Nonetheless we need to ensure all the gas cost in a reasonable range.
Based on that we've added a Configuring Extrinsic for all the factors above. And also there are two smoke tests added accordingly.