Refactoring XcmExportFeeToSibling and more tests for outbound fee #1109

yrong · 2024-01-11T11:32:57Z

Resolves: SNO-820, SNO-812

This is the sibling PR for #1107 without the change required for the xcm primitives in Snowfork/polkadot-sdk#95.

In this PR we focus on

adding more sanity checks for the params and more tests accordingly
improving the error log for XcmExportFeeToSibling

codecov · 2024-01-11T11:53:08Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (e0506f7) 76.36% compared to head (15132aa) 76.25%.
Report is 7 commits behind head on main.

Files	Patch %	Lines
parachain/runtime/runtime-common/src/lib.rs	62.16%	14 Missing ⚠️
parachain/pallets/ethereum-client/src/impls.rs	0.00%	6 Missing ⚠️
parachain/runtime/test-common/src/lib.rs	0.00%	6 Missing ⚠️
parachain/pallets/system/src/lib.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1109      +/-   ##
==========================================
- Coverage   76.36%   76.25%   -0.12%     
==========================================
  Files          56       58       +2     
  Lines        2344     2400      +56     
  Branches       72       72              
==========================================
+ Hits         1790     1830      +40     
- Misses        537      553      +16     
  Partials       17       17

Flag	Coverage Δ
rust	`75.37% <49.05%> (-0.12%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alistair-singh

Looks good. Added minor comments.

parachain/primitives/core/src/lib.rs

parachain/runtime/runtime-common/src/lib.rs

Co-authored-by: Alistair Singh <[email protected]>

… into ron/handle-fee

vgeddes · 2024-01-16T11:47:14Z

parachain/pallets/outbound-queue/src/send_message_impl.rs

+		ensure!(
+			gas_used_at_most > T::GasMeter::MAXIMUM_BASE_GAS &&
+				gas_used_at_most < T::GasMeter::MAXIMUM_GAS_CAP,
+			SendError::MessageInvalidGasFees
+		);
 		let fee = Self::calculate_fee(gas_used_at_most, T::PricingParameters::get());
+		let zero_fee = <Self as SendMessageFeeProvider>::Balance::zero();
+		ensure!(fee.local > zero_fee && fee.remote > zero_fee, SendError::MessageInvalidGasFees);


These checks don't make sense to me. The expectation is that T::GasMeter::maximum_gas_used_at_most and Self::calculate_fee always return correct values for any arbitrary Command.

This behavior is important, as we can't reject commands because for some reason we could not calculate fees. To do otherwise would lead to a loss of user funds.

It seems these checks are acting as unit tests, and so we should convert them to actual unit tests.

This check fee.remote > 0 is for the test case as I mentioned here in #1109 (comment) in which the remote_fee calculated being zero(though pricing parameters are all non-zero and valid). It will lead to the sovereign account of the sibling chain not being properly refunded in XcmExportFeeToSibling and finally the agent on the Ethereum side can't be funded.

That's also the reason why I introduced the sanity check of the pricing params, prefer to reject commands because for some reason we could not calculate fees rather than silently ignoring.

MAXIMUM_GAS_CAP added here is a hard limit that applies to all commands. It's mainly for command Upgrade with a sanity initializer_max_gas. It also applies to the future arbitrary transact for which the gas is dynamic from the input of the end-user.

MAXIMUM_GAS_CAP should be consistent with the default relayer config otherwise the check in Gateway will always fail and the bridge stuck.

This check fee.remote > 0 is for the test case as I mentioned here in #1109 (comment) in which the remote_fee calculated being zero(though pricing parameters are all non-zero and valid). It will lead to the sovereign account of the sibling chain not being properly refunded in XcmExportFeeToSibling and finally the agent on the Ethereum side can't be funded.

Thanks Ron, that makes sense. But it seems there is a bug in our code if the calculated remote fee ends up being zero. Can you investigate why this happens? We should try and fix it so that we don't need to have these checks.

I don't think we should introduce MAXIMUM_GAS_CAP, for the same reasons I outlined in #1109 (comment).

See #1109 (comment). I think its fine if calculated remote fee is zero. It just means the fee is too low to be represented using DOT decimals.

Checks reverted in af15053

parachain/primitives/core/src/outbound.rs

parachain/primitives/core/src/pricing.rs

vgeddes · 2024-01-16T12:07:00Z

parachain/primitives/core/src/pricing.rs

+		if self.exchange_rate < FixedU128::from_rational(1, 10000) ||
+			self.exchange_rate > FixedU128::from_rational(1, 1)
+		{


I prefer not to change the validation logic. I also thought about this approach, but ultimately decided it wasn't worth it.

If the exchange rate does actually mistakenly get set to 1/9999 (slightly less than the limit), then that practically still ends up blocking the bridge anyway.

The upper limit is arbitrary, and if the real-world exchange rate breaches the limit (quite possible), then a runtime upgrade would be required to fix the validation code.

Reverted in f43fce3

My only concern is #1109 (comment) but can be avoided by double check before sending the governance call so should be fine.

parachain/primitives/core/src/pricing.rs

parachain/runtime/runtime-common/src/tests.rs

parachain/runtime/test-common/src/lib.rs

parachain/pallets/outbound-queue/src/test.rs

parachain/pallets/system/src/tests.rs

parachain/primitives/router/src/outbound/mod.rs

parachain/runtime/runtime-common/src/lib.rs

parachain/runtime/runtime-common/src/tests.rs

vgeddes

+1

yrong added 10 commits January 10, 2024 22:47

Handle fee with error instead of silently failing

3ac5f12

Merge branch 'main' into ron/handle-fee-with-error

319d4ba

Fix for pallet rename

279dbf2

AssetTransactor::deposit_asset directly with error thrown

7f7e4bc

Move invalid params to test

4ddca9c

Check more strictly

965e579

More tests

5fb402b

More tests

3894746

Handle fee without error

8361b26

Update sdk

79f79f1

yrong mentioned this pull request Jan 11, 2024

Handle fee with error instead of silently ignoring #1107

Closed

yrong marked this pull request as ready for review January 11, 2024 12:00

yrong added 2 commits January 11, 2024 20:58

More tests for outbound fee

3fa6fcd

More tests

76f4d4f

alistair-singh approved these changes Jan 12, 2024

View reviewed changes

yrong and others added 6 commits January 12, 2024 16:00

Update parachain/runtime/runtime-common/src/lib.rs

d359e19

Co-authored-by: Alistair Singh <[email protected]>

Update parachain/runtime/runtime-common/src/lib.rs

6ca0929

Co-authored-by: Alistair Singh <[email protected]>

Update parachain/runtime/runtime-common/src/lib.rs

5922a5e

Co-authored-by: Alistair Singh <[email protected]>

Update parachain/runtime/runtime-common/src/lib.rs

122a5a2

Co-authored-by: Alistair Singh <[email protected]>

Merge branch 'main' into ron/handle-fee

986f19e

Merge branch 'ron/handle-fee' of https://github.com/Snowfork/snowbridge…

c789e69

… into ron/handle-fee

yrong requested review from vgeddes and claravanstaden January 15, 2024 10:15

More param checks & more tests

7514bd1

yrong changed the title ~~Improve handle fee with more tests and logs~~ Sanity checks for the pricing params & Improve XcmExportFeeToSibling with more tests and logs Jan 16, 2024

vgeddes reviewed Jan 16, 2024

View reviewed changes

yrong added 3 commits January 17, 2024 00:25

Rename test case less confusing

18daf99

Remove sanity check for Pricing params

f43fce3

Remove test unnecessary

711720c

yrong commented Jan 16, 2024

View reviewed changes

parachain/pallets/outbound-queue/src/test.rs Show resolved Hide resolved

claravanstaden reviewed Jan 16, 2024

View reviewed changes

Revert to unroutable

de03393

yrong changed the title ~~Sanity checks for the pricing params & Improve XcmExportFeeToSibling with more tests and logs~~ Improve XcmExportFeeToSibling with more tests and logs Jan 17, 2024

yrong added 2 commits January 17, 2024 12:40

Merge branch 'main' into ron/handle-fee

85cb976

Revert the checks

af15053

yrong force-pushed the ron/handle-fee branch from e4092d6 to af15053 Compare January 17, 2024 12:37

Rename as MockAssetTransactor

122572d

yrong changed the title ~~Improve XcmExportFeeToSibling with more tests and logs~~ Refactoring XcmExportFeeToSibling with logs/tests and add some more tests for calculating the outbound fee Jan 18, 2024

yrong changed the title ~~Refactoring XcmExportFeeToSibling with logs/tests and add some more tests for calculating the outbound fee~~ Refactoring XcmExportFeeToSibling with logs/tests and more tests for the outbound fee Jan 18, 2024

yrong changed the title ~~Refactoring XcmExportFeeToSibling with logs/tests and more tests for the outbound fee~~ Refactoring XcmExportFeeToSibling and more tests for outbound fee Jan 18, 2024

vgeddes approved these changes Jan 18, 2024

View reviewed changes

Update sdk

15132aa

yrong merged commit 4e24732 into main Jan 19, 2024
7 checks passed

yrong deleted the ron/handle-fee branch January 19, 2024 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring XcmExportFeeToSibling and more tests for outbound fee #1109

Refactoring XcmExportFeeToSibling and more tests for outbound fee #1109

yrong commented Jan 11, 2024 •

edited

Loading

codecov bot commented Jan 11, 2024 •

edited

Loading

alistair-singh left a comment

vgeddes Jan 16, 2024 •

edited

Loading

yrong Jan 17, 2024 •

edited

Loading

yrong Jan 17, 2024 •

edited

Loading

vgeddes Jan 17, 2024 •

edited

Loading

vgeddes Jan 17, 2024

vgeddes Jan 17, 2024

yrong Jan 17, 2024

vgeddes Jan 16, 2024

yrong Jan 16, 2024 •

edited

Loading

vgeddes left a comment

Refactoring XcmExportFeeToSibling and more tests for outbound fee #1109

Refactoring XcmExportFeeToSibling and more tests for outbound fee #1109

Conversation

yrong commented Jan 11, 2024 • edited Loading

codecov bot commented Jan 11, 2024 • edited Loading

Codecov Report

alistair-singh left a comment

Choose a reason for hiding this comment

vgeddes Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

yrong Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

yrong Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

vgeddes Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

vgeddes Jan 17, 2024

Choose a reason for hiding this comment

vgeddes Jan 17, 2024

Choose a reason for hiding this comment

yrong Jan 17, 2024

Choose a reason for hiding this comment

vgeddes Jan 16, 2024

Choose a reason for hiding this comment

yrong Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

vgeddes left a comment

Choose a reason for hiding this comment

yrong commented Jan 11, 2024 •

edited

Loading

codecov bot commented Jan 11, 2024 •

edited

Loading

vgeddes Jan 16, 2024 •

edited

Loading

yrong Jan 17, 2024 •

edited

Loading

yrong Jan 17, 2024 •

edited

Loading

vgeddes Jan 17, 2024 •

edited

Loading

yrong Jan 16, 2024 •

edited

Loading