feature/PRO-1038/pool-fee-rpc #4459

Janislav · 2024-01-26T15:22:28Z

Pull Request

Closes: PRO-xxx

Checklist

Please conduct a thorough self-review before opening the PR.

I am confident that the code works.
I have updated documentation where appropriate.

Summary

Please include a succinct description of the purpose and content of the PR. What problem does it solve, and how? Link issues, discussions, other PRs, and anything else that will help the reviewer.

codecov · 2024-01-26T15:31:18Z

Codecov Report

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

Comparison is base (cb12b67) 73% compared to head (3864f7e) 73%.
Report is 6 commits behind head on main.

❗ Current head 3864f7e differs from pull request most recent head ccc1415. Consider uploading reports for the commit ccc1415 to get more accurate results

Files	Patch %	Lines
state-chain/runtime/src/lib.rs	18%	0 Missing and 9 partials ⚠️
state-chain/pallets/cf-pools/src/lib.rs	88%	0 Missing and 1 partial ⚠️
state-chain/pallets/cf-pools/src/mock.rs	92%	0 Missing and 1 partial ⚠️
state-chain/traits/src/mocks/lp_balance.rs	0%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #4459    +/-   ##
======================================
- Coverage     73%     73%    -0%     
======================================
  Files        401     397     -4     
  Lines      68209   67846   -363     
  Branches   68209   67846   -363     
======================================
- Hits       49617   49221   -396     
- Misses     15964   15988    +24     
- Partials    2628    2637     +9

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

Janislav · 2024-01-31T13:38:54Z

Docs are updated here: chainflip-io/chainflip-docs#181

Janislav · 2024-02-01T14:26:25Z

state-chain/pallets/cf-pools/src/lib.rs

@@ -1157,6 +1157,14 @@ pub struct PoolInfo {
 	/// The fee taken, when range orders are used, from swap inputs that contributes to liquidity
 	/// provider earnings
 	pub range_order_fee_hundredth_pips: u32,
+	/// The total fees earned in this pool by range orders.
+	pub range_order_total_fees_earned: SideMap<Amount>,


Not sure about the return value here. Maybe something scalar would make more sense? Same for all the over new added fields.

So we can use SideMap as you have here, and in the other PR of yours we will change it to PairMap (or whatever we call it).

AlastairHolmes · 2024-02-08T10:56:54Z

So the fees aren't all taken in a single asset, so HistoricalEarnedFees needs to be a double map.

Janislav · 2024-02-08T15:12:41Z

Okay, interesting. I somehow remembered that we only collect LP fees in USDC. Probably this has changed in the meantime.

AlastairHolmes · 2024-02-08T15:44:31Z

Okay, interesting. I somehow remembered that we only collect LP fees in USDC. Probably this has changed in the meantime.

thats the network fee

Janislav · 2024-02-08T15:56:11Z

Yeah, I know that. I think in the very early design (first Whitewater), it was like that 🤔. Anyway - ill change it 😅

AlastairHolmes · 2024-02-19T09:38:57Z

state-chain/runtime/src/runtime_apis.rs

@@ -75,6 +75,7 @@ pub struct AuctionState {
 pub struct LiquidityProviderInfo {
 	pub refund_addresses: Vec<(ForeignChain, Option<ForeignChainAddress>)>,
 	pub balances: Vec<(Asset, AssetAmount)>,
+	pub earned_fees: BTreeMap<ForeignChain, Vec<(Asset, AssetAmount)>>,


Can you use the new any::AssetMap?

AlastairHolmes · 2024-02-19T09:40:12Z

state-chain/pallets/cf-lp/src/lib.rs

@@ -139,6 +139,11 @@ pub mod pallet {
 	pub type FreeBalances<T: Config> =
 		StorageDoubleMap<_, Twox64Concat, T::AccountId, Identity, Asset, AssetAmount>;

+	#[pallet::storage]
+	/// Historical earned fees for an account. Map: AccountId => AssetAmount
+	pub type HistoricalEarnedFees<T: Config> =


Actually we could use the AssetMap here too.

And use a StorageMap instead.

AlastairHolmes · 2024-02-19T09:48:07Z

state-chain/pallets/cf-pools/src/lib.rs

-			asset_pair.assets()[(!order.to_sold_side()).into()],
-			collected_fees,
-		)?;
+		let asset = asset_pair.assets()[(!order.to_sold_side()).into()];


You need to add the recording of fees into inner_update_range_order too.

I think I do this.

AlastairHolmes

Ok, sorry that took a while, once all the comments I've made are addressed, I'm happy for this to be merged.

Janislav · 2024-02-19T16:11:15Z

state-chain/pallets/cf-pools/src/lib.rs

@@ -1520,6 +1528,7 @@ impl<T: Config> Pallet<T> {
 				.try_map(|(asset, collected_fees)| {
 					AssetAmount::try_from(collected_fees).map_err(Into::into).and_then(
 						|collected_fees| {
+							T::LpBalance::record_fees(lp, collected_fees, asset);


@AlastairHolmes that's the place where I have to add record the fees for range orders, right?

Sorry not sure how I could have missed that

…ero-liquidity * origin/main: (28 commits) feat(custom-rpc): add broker info [WEB-925] (#4560) chore: upgrade solana version (#4567) fix: continuous adapter (PRO-684) (#4503) fix: Wait for ThresholdSignature success before switching to NewKeysActivated (#4534) chore: remove un-needed serde derives from EncodedAddress (#4565) fix: more lenient max deposit fee in bouncer test (#4564) chore: build persa bins instead of fetch (#4554) feat: deploy L2 contracts upon localnet startup and send L2 TXs 📑 (#4558) feat: debug logs on runtime upgrade test (#4556) feature/PRO-1038/pool-fee-rpc (#4459) chore: update bootnodes in chainspec ✨ (#4456) fix: allow test coverage to run (#4555) refactor/pro-1160/remove-side-side-map (#4489) fix: activate missing migrations (#4550) chore: remove cf-build (#4551) feat: extensible multi-key rotator (#4546) fix: ensure channel open fee can be paid in benchmarks (#4544) feat: bump substrate deps to polkadot-sdk 1.6 (#4504) fix: upgrade-test should restart the chainflip-nodes on an incompatible upgrade (#4490) test: add small v3 migration test (#4543) ... # Conflicts: # state-chain/pallets/cf-pools/src/tests.rs

…-price * origin/main: feat(custom-rpc): add broker info [WEB-925] (#4560) chore: upgrade solana version (#4567) fix: continuous adapter (PRO-684) (#4503) fix: Wait for ThresholdSignature success before switching to NewKeysActivated (#4534) chore: remove un-needed serde derives from EncodedAddress (#4565) fix: more lenient max deposit fee in bouncer test (#4564) chore: build persa bins instead of fetch (#4554) feat: deploy L2 contracts upon localnet startup and send L2 TXs 📑 (#4558) feat: debug logs on runtime upgrade test (#4556) feature/PRO-1038/pool-fee-rpc (#4459) chore: update bootnodes in chainspec ✨ (#4456) fix: allow test coverage to run (#4555) refactor/pro-1160/remove-side-side-map (#4489) fix: activate missing migrations (#4550) chore: remove cf-build (#4551) # Conflicts: # state-chain/pallets/cf-pools/src/lib.rs

Janislav added 5 commits January 23, 2024 15:56

feat: ✨ added total_fees_earned and total_swap_inputs to rpc

6fe6506

Merge branch 'main' into feature/pro-1038/pool-fee-rpc

c3d5d82

added current work in progress

c5dff7c

feat: ✨ added recording of all time earned fees per lp

12bac95

test: 🧪 added test for fee recording 🎤

61d9700

Janislav added 3 commits January 29, 2024 11:02

chore: removed out comment code, added description to struct fields

9e74b9c

test: 🧪 test fees in integration test

7eeb324

Merge branch 'main' into feature/pro-1038/pool-fee-rpc

2568183

Janislav marked this pull request as ready for review January 31, 2024 13:39

Janislav requested a review from a team as a code owner January 31, 2024 13:39

Janislav requested review from jerryafr, acdibble and AlastairHolmes and removed request for a team January 31, 2024 13:39

Janislav commented Feb 1, 2024

View reviewed changes

Merge branch 'main' into feature/pro-1038/pool-fee-rpc

4229b52

Janislav added 2 commits February 8, 2024 17:28

wip

7c3c4ff

refactor: ♻️ made HistoricalEarnedFees a DoubleMap

470a52d

Janislav requested a review from dandanlen as a code owner February 9, 2024 16:03

Merge branch 'main' into feature/pro-1038/pool-fee-rpc

3864f7e

AlastairHolmes reviewed Feb 19, 2024

View reviewed changes

AlastairHolmes approved these changes Feb 19, 2024

View reviewed changes

Merge branch 'main' into feature/pro-1038/pool-fee-rpc

a4a1a29

Janislav commented Feb 19, 2024

View reviewed changes

Janislav added 6 commits February 20, 2024 15:09

refactor: ♻️ using AssetMap now

c46986d

Merge branch 'main' into feature/pro-1038/pool-fee-rpc

d18d671

fix: 🚨 clippy

2ed5470

Merge branch 'main' into feature/pro-1038/pool-fee-rpc

8c63160

chore: set pool fee back to 0

10c25c6

Merge branch 'main' into feature/pro-1038/pool-fee-rpc

ccc1415

Janislav merged commit 38f4443 into main Feb 21, 2024
41 of 42 checks passed

Janislav deleted the feature/pro-1038/pool-fee-rpc branch February 21, 2024 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/PRO-1038/pool-fee-rpc #4459

feature/PRO-1038/pool-fee-rpc #4459

Janislav commented Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading

Janislav commented Jan 31, 2024

Janislav Feb 1, 2024 •

edited

Loading

AlastairHolmes Feb 19, 2024

AlastairHolmes commented Feb 8, 2024

Janislav commented Feb 8, 2024

AlastairHolmes commented Feb 8, 2024

Janislav commented Feb 8, 2024 •

edited

Loading

AlastairHolmes Feb 19, 2024

AlastairHolmes Feb 19, 2024

AlastairHolmes Feb 19, 2024

AlastairHolmes Feb 19, 2024

Janislav Feb 19, 2024

AlastairHolmes left a comment

Janislav Feb 19, 2024

AlastairHolmes Feb 20, 2024

feature/PRO-1038/pool-fee-rpc #4459

feature/PRO-1038/pool-fee-rpc #4459

Conversation

Janislav commented Jan 26, 2024 • edited Loading

Pull Request

Checklist

Summary

codecov bot commented Jan 26, 2024 • edited Loading

Codecov Report

Janislav commented Jan 31, 2024

Janislav Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlastairHolmes commented Feb 8, 2024

Janislav commented Feb 8, 2024

AlastairHolmes commented Feb 8, 2024

Janislav commented Feb 8, 2024 • 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

AlastairHolmes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janislav commented Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading

Janislav Feb 1, 2024 •

edited

Loading

Janislav commented Feb 8, 2024 •

edited

Loading