feat: shave fees on ingress #4335

dandanlen · 2023-12-08T16:44:07Z

Closes: PRO-1047

state-chain/chains/src/btc.rs

state-chain/chains/src/dot.rs

state-chain/pallets/cf-ingress-egress/src/lib.rs

state-chain/chains/src/eth.rs

state-chain/chains/src/dot.rs

state-chain/chains/src/eth.rs

state-chain/pallets/cf-ingress-egress/src/lib.rs

- set tracked fees to zero - handle edge cases in asset conversion - return remaining input instead of converted input

state-chain/cf-integration-tests/src/mock_runtime.rs

state-chain/chains/src/dot.rs

codecov · 2023-12-11T22:13:12Z

Codecov Report

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

Comparison is base (e8fd491) 71% compared to head (eadb16a) 71%.
Report is 4 commits behind head on main.

Files	Patch %	Lines
state-chain/runtime/src/lib.rs	8%	0 Missing and 44 partials ⚠️
state-chain/custom-rpc/src/lib.rs	59%	15 Missing ⚠️
state-chain/traits/src/mocks/asset_converter.rs	50%	12 Missing and 2 partials ⚠️
state-chain/chains/src/btc.rs	0%	13 Missing ⚠️
state-chain/chains/src/mocks.rs	0%	11 Missing ⚠️
state-chain/pallets/cf-pools/src/lib.rs	84%	6 Missing and 3 partials ⚠️
state-chain/chains/src/lib.rs	0%	6 Missing ⚠️
state-chain/pallets/cf-ingress-egress/src/lib.rs	97%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #4335    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        390     393     +3     
  Lines      66407   66667   +260     
  Branches   66407   66667   +260     
======================================
+ Hits       47451   47497    +46     
- Misses     16580   16740   +160     
- Partials    2376    2430    +54

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

dandanlen

I'll do another pass tomorrow.

state-chain/chains/src/mocks.rs

state-chain/pallets/cf-ingress-egress/src/lib.rs

state-chain/primitives/src/chains.rs

state-chain/pallets/cf-ingress-egress/src/lib.rs

AlastairHolmes · 2023-12-12T12:54:47Z

state-chain/chains/src/btc.rs

+		_asset: <Bitcoin as Chain>::ChainAsset,
+	) -> <Bitcoin as Chain>::ChainAmount {
+		// Include the min fee so we over-estimate the cost.
+		self.btc_fee_info.min_fee_required_per_tx + self.btc_fee_info.fee_per_input_utxo


min_fee_required_per_tx is not an overestimate as we assume 10 bytes for this min, but the maximum amount of bytes in a transaction that isn't associated with an input/output, seems to be 26 bytes (https://en.bitcoin.it/wiki/Transaction). So we could under-estimate if there are large numbers of inputs or outputs.

I'm not clear on the reason a fee numbers need to be taken on ingress here. I think a comment would be helpful as the reasoning for the fees being needed is different for each chain.

In the Eth case, we take fees in the ingress case because we do a fetch into the vault. And someone has to pay for that.

But in the Btc case, we take fees to pay for the cost of including the UXTO, in some future egress.

Also I believe the min_fee_required_per_tx is actually not needed, as you are basically double counting it in the estimate_egress_fee.

As discussed, I agree that we are double-counting the min_fee_required_per_tx but this is deliberate to ensure we overestimate the fees. I agree there is plenty of room for improvement. I'll add some comments that hopefully make some of this a bit clearer.

I will add these comments separately if that's ok - have them prepared but forgot to push the commit. And I don't want to push it now and delay the PR anoother 40 minutes while CI completes.

AlastairHolmes · 2023-12-12T13:31:58Z

state-chain/pallets/cf-ingress-egress/src/lib.rs

+							IngressOrEgress::Egress,
+							asset,
+							amount,
+						),
 						to: destination_address,
 					});
 					DepositBalances::<T, I>::mutate(asset, |tracker| {


Believe you should be passing the result from withhold_transaction_fee here.

On reflection I'm not so sure - the fee is no longer in a deposit (ie. available to be spent), it's now in the withheld assets, which are earmarked for paying fees.

I think if we do what alastair suggested, we also need to account for it in the other places where the deposit tracker is mutated. So basically the mark_as_fetched and the register_deposit functions should also take the net amount for accounting to be correct. Alternatively, we can leave it as as and include the fee in the accounting at all places DepositTracker is mutated.

I think leaving it as is probably the best option. It would still do its job of tracking the deposits (kind off). Doing it the right way would require non-trivial changes.

As mentioned in Discord - I think it's correct as is. The withheld assets should be considered 'spent' ie. they are no longer ours.

…4339) * feat: add ingress/egress fees to cf_ingress_egress_environment rpc * chore: use named Instances --------- Co-authored-by: kylezs <zsembery.kyle@gmail.com>

* feat: shave fees on ingress * feat: withhold tx fees * fix: adjust fee calculations * feat: withhold ingress and egress fee on ingress * fix: basic mock implementations * fix: missing import * fix: eth fetch doens't incur transfer cost * feat: withhold fees in correct asset * fix: more accurate eth fee calculations * feat: shave egress fee at egress * feat: instant asset conversion for tx fees * fix: typo * fix: get integration tests to pass: - set tracked fees to zero - handle edge cases in asset conversion - return remaining input instead of converted input * fix: withhold fees in gas asset * fix: doc comments * fix: correct polkadot fees * chore: clippy * fix bouncer * fix: better output amount estimation method * feat: a unit test * feat: add ingress/egress fees to cf_ingress_egress_environment rpc (#4339) * feat: add ingress/egress fees to cf_ingress_egress_environment rpc * chore: use named Instances --------- Co-authored-by: kylezs <zsembery.kyle@gmail.com> --------- Co-authored-by: Martin Rieke <martin@chainflip.io> Co-authored-by: kylezs <zsembery.kyle@gmail.com>

…-timeout * origin/main: feat: end to end network upgrade github action (#4274) feat: spec_version of PR is greater than spec version of current release (#4355) feat: enforce version is greater than release version on PRs to main (#4351) fix: btc witnesser test failing sometimes (#4353) fix: connections can become stale when reconnecting (#4310) chore: add `chainflip-rpc-node` systemd file 🚀 (#4352) feat: API Bins check SC compatibility (#4342) chore: update runtime spec checks ⛓️ (#4349) feat: Add version cmd to all bins (#4343) fix: changelog check 🤫 (#4348) chore: update docker tags 🐳 (#4347) chore: add runtime version check 👀 (#4344) feat: shave fees on ingress (#4335) pick/persa fixes (#4329) feat: track btc fees on success (#4334) ensure we dont create BTC transaction outputs below the bitcoin dust limit (#4340) fix: sweeping before withdrawal (#4337)

dandanlen commented Dec 8, 2023

View reviewed changes

state-chain/chains/src/btc.rs Show resolved Hide resolved

dandanlen commented Dec 8, 2023

View reviewed changes

state-chain/chains/src/dot.rs Outdated Show resolved Hide resolved

dandanlen commented Dec 8, 2023

View reviewed changes

state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved

dandanlen commented Dec 8, 2023

View reviewed changes

state-chain/chains/src/eth.rs Show resolved Hide resolved

dandanlen requested review from kylezs, ramizhasan111 and martin-chainflip December 11, 2023 14:01

ramizhasan111 reviewed Dec 11, 2023

View reviewed changes

state-chain/chains/src/dot.rs Outdated Show resolved Hide resolved

dandanlen requested a review from albert-llimos December 11, 2023 14:46

ramizhasan111 reviewed Dec 11, 2023

View reviewed changes

state-chain/chains/src/eth.rs Show resolved Hide resolved

albert-llimos reviewed Dec 11, 2023

View reviewed changes

state-chain/chains/src/eth.rs Show resolved Hide resolved

state-chain/chains/src/eth.rs Outdated Show resolved Hide resolved

ramizhasan111 reviewed Dec 11, 2023

View reviewed changes

state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved

dandanlen force-pushed the feat/shave-fees-on-ingress branch from e1a1103 to 7eb8882 Compare December 11, 2023 16:57

dandanlen commented Dec 11, 2023

View reviewed changes

state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved

dandanlen added 14 commits December 11, 2023 23:05

feat: shave fees on ingress

3d3c93c

feat: withhold tx fees

642a7ee

fix: adjust fee calculations

499130d

feat: withhold ingress and egress fee on ingress

2db03d0

fix: basic mock implementations

18001ed

fix: missing import

dd1b52a

fix: eth fetch doens't incur transfer cost

c84bdbe

feat: withhold fees in correct asset

78e394e

fix: more accurate eth fee calculations

7daa0b3

feat: shave egress fee at egress

a79ee83

feat: instant asset conversion for tx fees

4e7b462

fix: typo

171d7c8

fix: get integration tests to pass:

99d3c46

- set tracked fees to zero - handle edge cases in asset conversion - return remaining input instead of converted input

fix: withhold fees in gas asset

6c12a4f

dandanlen force-pushed the feat/shave-fees-on-ingress branch from 44479eb to 6c12a4f Compare December 11, 2023 22:05

dandanlen commented Dec 11, 2023

View reviewed changes

state-chain/cf-integration-tests/src/mock_runtime.rs Show resolved Hide resolved

dandanlen commented Dec 11, 2023

View reviewed changes

state-chain/cf-integration-tests/src/mock_runtime.rs Show resolved Hide resolved

dandanlen commented Dec 11, 2023

View reviewed changes

state-chain/chains/src/dot.rs Outdated Show resolved Hide resolved

dandanlen commented Dec 11, 2023

View reviewed changes

dandanlen added 3 commits December 12, 2023 07:56

fix: doc comments

2fa70f1

fix: correct polkadot fees

c1b2432

chore: clippy

9d625ee

ramizhasan111 reviewed Dec 12, 2023

View reviewed changes

state-chain/pallets/cf-ingress-egress/src/lib.rs Show resolved Hide resolved

AlastairHolmes reviewed Dec 12, 2023

View reviewed changes

martin-chainflip and others added 4 commits December 12, 2023 14:57

fix bouncer

3f4e634

fix: better output amount estimation method

6e17c70

feat: a unit test

c2035e2

feat: add ingress/egress fees to cf_ingress_egress_environment rpc (#…

eadb16a

…4339) * feat: add ingress/egress fees to cf_ingress_egress_environment rpc * chore: use named Instances --------- Co-authored-by: kylezs <zsembery.kyle@gmail.com>

kylezs mentioned this pull request Dec 12, 2023

chore: pick release changes to 1.1 #4341

Merged

kylezs marked this pull request as ready for review December 12, 2023 18:02

kylezs requested a review from a team as a code owner December 12, 2023 18:02

kylezs requested review from jerryafr and niklasnatter and removed request for a team December 12, 2023 18:02

kylezs enabled auto-merge (squash) December 12, 2023 18:11

martin-chainflip approved these changes Dec 12, 2023

View reviewed changes

kylezs merged commit 2e0efa1 into main Dec 12, 2023
41 checks passed

kylezs deleted the feat/shave-fees-on-ingress branch December 12, 2023 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: shave fees on ingress #4335

feat: shave fees on ingress #4335

dandanlen commented Dec 8, 2023 •

edited by kylezs

Loading

codecov bot commented Dec 11, 2023 •

edited

Loading

dandanlen left a comment

AlastairHolmes Dec 12, 2023 •

edited

Loading

AlastairHolmes Dec 12, 2023 •

edited

Loading

AlastairHolmes Dec 12, 2023

dandanlen Dec 12, 2023

dandanlen Dec 12, 2023

AlastairHolmes Dec 12, 2023

dandanlen Dec 12, 2023

ramizhasan111 Dec 12, 2023

ramizhasan111 Dec 12, 2023

dandanlen Dec 12, 2023

feat: shave fees on ingress #4335

feat: shave fees on ingress #4335

Conversation

dandanlen commented Dec 8, 2023 • edited by kylezs Loading

codecov bot commented Dec 11, 2023 • edited Loading

Codecov Report

dandanlen left a comment

Choose a reason for hiding this comment

AlastairHolmes Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

AlastairHolmes Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandanlen commented Dec 8, 2023 •

edited by kylezs

Loading

codecov bot commented Dec 11, 2023 •

edited

Loading

AlastairHolmes Dec 12, 2023 •

edited

Loading

AlastairHolmes Dec 12, 2023 •

edited

Loading