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: auto sweep earnings and accurate free balance rpc (PRO-856) #4145

Merged
merged 2 commits into from Oct 23, 2023

Conversation

AlastairHolmes
Copy link
Contributor

@AlastairHolmes AlastairHolmes commented Oct 19, 2023

This makes it so the free balance rpc considers the fees that have not been collected yet, and the extrinsics sweep/collect all positions before doing anything. Thereby making it seem as if earnings are always returned directly to an lp's free balance.

@linear
Copy link

linear bot commented Oct 19, 2023

PRO-856 RPC for LP Free Balance

Needed for frontend LP app

RPC needs to account for balance that is currently held inside positions (i.e. fees that have been earned, but not collected).

@@ -1106,6 +1118,62 @@ pub struct UnidirectionalPoolDepth {
}

impl<T: Config> Pallet<T> {
fn inner_sweep(lp: &T::AccountId) -> DispatchResult {
// Collect to avoid undefined behaviour (See StorsgeMap::iter_keys documentation)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that iterating over the keys and modifying only the values should be ok (and in fact preferable from a memory usage POV), but I agree the documentation seems to suggest otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It specifically says it is undefined

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know but I looked at the code, and the implementation of translate which does basically this internally.

And depending on which of the hierarchy of traits you read the documentation for, it sometimes says 'if you alter the value' but sometimes 'if you add or remove a value'. The latter makes sense, the former not really (why would changing the value affect the iteration of keys?).

Feel free to leave as is, I was just adding some context.

.cloned()
.map(|limit_orders_cache| (side, limit_orders_cache))
})
.collect::<Vec<_>>()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this collect necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it will not compile otherwise, as pool is mutated inside the loop

Base automatically changed from fix/account-info-rpc to main October 23, 2023 14:49
@AlastairHolmes AlastairHolmes requested a review from a team as a code owner October 23, 2023 14:49
@AlastairHolmes AlastairHolmes requested review from jerryafr and niklasnatter and removed request for a team October 23, 2023 14:49
@AlastairHolmes AlastairHolmes enabled auto-merge (squash) October 23, 2023 15:14
@martin-chainflip
Copy link
Contributor

worth adding a test that demonstrates the new behaviour

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #4145 (d202ebb) into main (c54d754) will increase coverage by 0%.
The diff coverage is 23%.

@@          Coverage Diff          @@
##            main   #4145   +/-   ##
=====================================
  Coverage     72%     72%           
=====================================
  Files        378     378           
  Lines      60857   60921   +64     
  Branches   60857   60921   +64     
=====================================
+ Hits       43664   43744   +80     
+ Misses     14926   14904   -22     
- Partials    2267    2273    +6     
Files Coverage Δ
state-chain/pallets/cf-lp/src/lib.rs 69% <ø> (ø)
state-chain/pallets/cf-lp/src/mock.rs 80% <100%> (+<1%) ⬆️
state-chain/runtime/src/lib.rs 35% <0%> (-<1%) ⬇️
state-chain/traits/src/liquidity.rs 20% <0%> (-2%) ⬇️
state-chain/pallets/cf-pools/src/lib.rs 59% <82%> (+2%) ⬆️
api/lib/src/lp.rs 0% <0%> (ø)
state-chain/pallets/cf-lp/src/weights.rs 0% <0%> (ø)
state-chain/pallets/cf-pools/src/weights.rs 0% <0%> (ø)

... and 7 files with indirect coverage changes

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

@AlastairHolmes AlastairHolmes merged commit f9bf5e5 into main Oct 23, 2023
42 checks passed
@AlastairHolmes AlastairHolmes deleted the feat/sweeping-and-free-balance branch October 23, 2023 16:03
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.

None yet

3 participants