Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Set pool fees #4050

Merged
merged 16 commits into from
Oct 12, 2023
Merged

Feat: Set pool fees #4050

merged 16 commits into from
Oct 12, 2023

Conversation

syan095
Copy link
Contributor

@syan095 syan095 commented Sep 27, 2023

Pull Request

Closes: PRO-846

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

Added function to set liquidity pool fees.
Changing the pool fees will collect all fees and bought amount for LPer and credit them to Lpers' account.

Changing the pool fees will collect all fees and bought amount for LPer
and credit them to Lpers' account.
@linear
Copy link

linear bot commented Sep 27, 2023

PRO-846 feat: change pool fee

We need the ability to change the pool lp fee. Either we could do this through deleting the pool and recreating it, or via a set fee function (which internally will basically have to do the same).

@syan095 syan095 requested review from AlastairHolmes, dandanlen and ramizhasan111 and removed request for ramizhasan111 September 27, 2023 00:53
@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #4050 (d006c32) into main (6436088) will increase coverage by 0%.
Report is 5 commits behind head on main.
The diff coverage is 93%.

@@          Coverage Diff           @@
##            main   #4050    +/-   ##
======================================
  Coverage     71%     72%            
======================================
  Files        377     377            
  Lines      60078   60624   +546     
  Branches   60078   60624   +546     
======================================
+ Hits       42810   43485   +675     
+ Misses     15022   14880   -142     
- Partials    2246    2259    +13     
Files Coverage Δ
state-chain/amm/src/limit_orders.rs 89% <100%> (+12%) ⬆️
state-chain/amm/src/range_orders.rs 82% <100%> (+4%) ⬆️
state-chain/pallets/cf-pools/src/benchmarking.rs 100% <100%> (ø)
state-chain/pallets/cf-pools/src/mock.rs 98% <100%> (+2%) ⬆️
state-chain/runtime/src/runtime_apis.rs 83% <100%> (ø)
state-chain/amm/src/common.rs 88% <88%> (-<1%) ⬇️
state-chain/amm/src/lib.rs 58% <86%> (+11%) ⬆️
state-chain/custom-rpc/src/lib.rs 22% <0%> (+1%) ⬆️
state-chain/pallets/cf-pools/src/tests.rs 99% <100%> (+2%) ⬆️
state-chain/runtime/src/lib.rs 35% <0%> (-<1%) ⬇️
... and 3 more

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Added benchmarking for setting pool fees
Copy link
Contributor

@AlastairHolmes AlastairHolmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thansk 4 da speling corectons :)

state-chain/pallets/cf-pools/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-pools/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-pools/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-pools/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-pools/src/tests.rs Show resolved Hide resolved
Pool's limit orders are updated with the pool's limit order
Updated unit to test this logic
Copy link
Contributor

@AlastairHolmes AlastairHolmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just these final comments to handle, and we're good to go I think

Copy link
Contributor

@AlastairHolmes AlastairHolmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed something. We also need to output the LimitOrderUpdated events too, I think it would be best to factor out the crediting of assetss and deposit of the event into the function "update_limit_order_storage" and maybe change the name of the function.

@AlastairHolmes AlastairHolmes requested a review from a team as a code owner September 29, 2023 09:53
@AlastairHolmes AlastairHolmes requested review from niklasnatter and zoheb391 and removed request for a team September 29, 2023 09:53
@AlastairHolmes
Copy link
Contributor

I also pushed some minor some what related stuff, if you can have a look.

* origin/main:
  Feat: don't include dust btc amounts on rotation (#4063)
  chore: update dependency and config.toml for RUSTSEC-2023-0065 (#4066)
  fix: loop_select conditions (PRO-587) (#4061)
  chore: remove unused config items (#4064)
  feat: size limit for CCM (#4015)
  fix: warn -> info (#4060)
  Fix: correctly handle peer updates while waiting to reconnect (#4052)

# Conflicts:
#	state-chain/chains/src/lib.rs
@syan095
Copy link
Contributor Author

syan095 commented Oct 3, 2023

@AlastairHolmes
I've made some changes to address your PR comments. Since the original PositionInfo was not tracked at all (in both Pool and AMM), I don't know where I can get the info to fill up the "delta_amount" field in the event. I think in this case, the amount isn't changed for the limit order, so I've made the "increase_or_decrease" and "delta_amount" into a single Option, so this can be set to None for situations like this, where we are only collecting fees and bought_amount and not adjusting the position amount. I'm not sure if this will cause any trouble for the Production/front end. If you have time can you please give this one more look over to see if my changes make sense? thank you!

@AlastairHolmes
Copy link
Contributor

Two things can you inline "update_limit_order_storage", and also change the RangeOrderUpdated event so the handling of the increase and decrease case is the same, i.e. with an option pair. Thanks, then we can merge.

@syan095 syan095 merged commit 390e3d1 into main Oct 12, 2023
44 checks passed
@syan095 syan095 deleted the feat/pools-change-fee branch October 12, 2023 21:23
syan095 added a commit that referenced this pull request Oct 12, 2023
* origin/main:
  Feat: Set pool fees (#4050)
  fix: don't egress empty all_batch calls (#4102)
  fix: don't abort broadcast if signers are unavailable (#4104)
  feat: add restricted balances to AccountInfoV2 (#4048)
  feat: use snake case for lp api method names (#4108)
  feat: add expiry block to liquidity channel event (#4111)

# Conflicts:
#	state-chain/custom-rpc/src/lib.rs
#	state-chain/runtime/src/lib.rs
#	state-chain/runtime/src/runtime_apis.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants