feat: RPC call for amount_to_liquidity #3835

syan095 · 2023-08-14T05:05:30Z

Pull Request

Closes: PRO-111

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 RPC call for getting the amount to liquidity

linear · 2023-08-14T05:05:32Z

PRO-111 Feature: RPC call for Amm:: liquidity_to_amounts()

Should expose liquidity_to_amounts() in the AMM as a rpc call, so front end can convert liquidity into asset amounts

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

codecov · 2023-08-14T05:17:45Z

Codecov Report

Merging #3835 (ff623e7) into main (f929770) will decrease coverage by 0%.
The diff coverage is 66%.

@@          Coverage Diff           @@
##            main   #3835    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        363     363            
  Lines      55659   55763   +104     
  Branches   55659   55763   +104     
======================================
+ Hits       39397   39467    +70     
- Misses     14303   14327    +24     
- Partials    1959    1969    +10

Files Changed	Coverage Δ
state-chain/amm/src/lib.rs	`65% <ø> (ø)`
state-chain/amm/src/range_orders.rs	`89% <0%> (-<1%)`	⬇️
state-chain/custom-rpc/src/lib.rs	`0% <0%> (ø)`
state-chain/runtime/src/lib.rs	`40% <10%> (-1%)`	⬇️
state-chain/pallets/cf-pools/src/lib.rs	`64% <65%> (+<1%)`	⬆️
state-chain/pallets/cf-pools/src/tests.rs	`96% <100%> (+1%)`	⬆️
state-chain/runtime/src/runtime_apis.rs	`80% <100%> (+3%)`	⬆️

... and 4 files with indirect coverage changes

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

state-chain/custom-rpc/src/lib.rs

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

dandanlen · 2023-08-14T11:55:16Z

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

+			)
+			.unwrap(),
+			1u128,
+		);


Having said all of the above, i'm not sure this test is really necessary. We don't need to ensure the accuracy of the conversion (this is, or should be tested in the amm crate).

What we really care about here is whether the rpc works as expected, for example it shouldn't panic if there is no pool, or if the user submits an invalid tick range.

This test here only verifies that the pub function worked as intended, I'm sure the inner workings works, as it was already used and tested in the AMM.

I've tested the RPC with local testnet to ensure the RPC works and wouldn't crash when the input was invalid.

What I mean is that the exact value of the output is not really relevant to this test: we don't actually require that the error is within 1 unit. 1 is an arbitary limit (why not 2 or 10?).

This test is checking the inner details of the amm crate instead of testing this 'layer' of the rpc call. What this layer actually adds is: checking that the asset is valid, and passing the args correctly to the amm, passing the result (ok/err) back up the rpc call chain. If anything we should test this.

I've tested the RPC with local testnet

Even better if we can enforce this with a unit test :)

Ok I think i understand now.
Removed the test for the AMM function itself, and instead test the correct error is returned when pool doesn't exist and when tick range isn't valid

dandanlen · 2023-08-14T11:57:05Z

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

+				upper,
+				SideMap::from_array([unstable_amount.into(), stable_amount.into()]),
+			)
+			.ok()


We just swallow the error here. We could instead return something useful. It looks like there are two possible errors, either the pool doesn't exist, or the tick range is invalid.

I found it difficult to return the error from run time to RPC call, I tried anyhow::error, &'static str and DispatchError, none of the was very nice to use.
I ended up having a custom error message when "None" was returned from the pallet call, so it provides better information to the RPC user.

~~We already have Error variants for all possible amm errors. I'll show you what I mean.~~

Ok I tried using DispatchError and I can see it's annoying! I think returning the pallet's Error<T> type might be an option, but I played around and I think a small dedicated error enum is the simplest to work with.

Yeah using a new error enum is definitely the best approach!

…-amounts * origin/main: chore: add gas limits to gaslimit test (#3842) chore: no early collect on storage_map (#3830)

dandanlen

👍 Thanks @syan095

Co-authored-by: Daniel <daniel@chainflip.io> Co-authored-by: dandanlen <3168260+dandanlen@users.noreply.github.com>

This reverts commit dbfe96a.

* Revert "feat: RPC call for amount_to_liquidity (#3835)" This reverts commit dbfe96a. * rename collected structures * Add PositionInfo squash * cut out inner_liquidity_to_amounts squash squash * cut out inner_amounts_to_liquidity * amm range orders mint and burn by Size * output operation liquidity delta * remove unneeded trait impls * move sqrt_price_to_price * current_price rpc * add nicer swap and position functions * remove pool rpcs * remove deser and ser impls * amm pool new function * current_price doesn't use traits * make private/hide unneeded interfaces * pub amount in PositionInfo * cutout error translation functions * desired -> maximum * rename RangeOrderSize to OldRangeOrderSize * new interfaces * new implementations * handle order ids * spelling and add safe mode checks * update bouncer for new lp-interface * bouncer linting * fix range_order bouncer command * bouncer linting * lp api returns * required asset ratio rpc * lp rpc * Use "order" not "position" * use u256 * utilities squash squash * use new utilities * pools function pool_info and pools functions * pool_orders * pool_range_order_liquidity_value * rpcs * _asset * fix * move debit down * bought_amount * LiquidityTooLarge * remove old test * remove range_order iszero check * basic comments --------- Co-authored-by: Roy Yang <roy@chainflip.io> Co-authored-by: Martin Rieke <martin@chainflip.io>

Added RPC call for getting amount to liquidity

6c7b2cb

syan095 requested a review from a team as a code owner August 14, 2023 05:05

syan095 requested review from niklasnatter and acdibble and removed request for a team August 14, 2023 05:05

syan095 requested a review from dandanlen August 14, 2023 05:05

syan095 commented Aug 14, 2023

View reviewed changes

state-chain/pallets/cf-pools/src/tests.rs Outdated Show resolved Hide resolved

dandanlen reviewed Aug 14, 2023

View reviewed changes

syan095 and others added 7 commits August 15, 2023 13:26

Address some PR comments

032fe1a

Fixed how errors are handled.

de0eec0

Merge remote-tracking branch 'origin/main' into feat/rpc-liquidity-to…

ff7dd7e

…-amounts * origin/main: chore: add gas limits to gaslimit test (#3842) chore: no early collect on storage_map (#3830)

fix: typo

4b596c1

feat: strong error type

3fcc9d8

Merge branch 'main' into feat/rpc-liquidity-to-amounts

a6f4a98

Improved unit test for getting liquidity from range order

d149440

syan095 requested a review from dandanlen August 15, 2023 22:47

dandanlen approved these changes Aug 17, 2023

View reviewed changes

Merge branch 'main' into feat/rpc-liquidity-to-amounts

ff623e7

dandanlen changed the title ~~Feature: RPC call for amount_to_liquidity~~ feat: RPC call for amount_to_liquidity Aug 17, 2023

dandanlen enabled auto-merge (squash) August 17, 2023 16:10

dandanlen merged commit dbfe96a into main Aug 17, 2023

dandanlen deleted the feat/rpc-liquidity-to-amounts branch August 17, 2023 16:43

marcellorigotti pushed a commit that referenced this pull request Aug 22, 2023

feat: RPC call for amount_to_liquidity (#3835)

48004ae

Co-authored-by: Daniel <daniel@chainflip.io> Co-authored-by: dandanlen <3168260+dandanlen@users.noreply.github.com>

AlastairHolmes pushed a commit that referenced this pull request Aug 23, 2023

Revert "feat: RPC call for amount_to_liquidity (#3835)"

bbc9bc1

This reverts commit dbfe96a.

AlastairHolmes pushed a commit that referenced this pull request Aug 23, 2023

Revert "feat: RPC call for amount_to_liquidity (#3835)"

b778acf

This reverts commit dbfe96a.

AlastairHolmes pushed a commit that referenced this pull request Aug 24, 2023

Revert "feat: RPC call for amount_to_liquidity (#3835)"

fc251dc

This reverts commit dbfe96a.

AlastairHolmes pushed a commit that referenced this pull request Aug 25, 2023

Revert "feat: RPC call for amount_to_liquidity (#3835)"

3a8c7da

This reverts commit dbfe96a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: RPC call for amount_to_liquidity #3835

feat: RPC call for amount_to_liquidity #3835

syan095 commented Aug 14, 2023 •

edited

Loading

linear bot commented Aug 14, 2023

codecov bot commented Aug 14, 2023 •

edited

Loading

dandanlen Aug 14, 2023

syan095 Aug 15, 2023

dandanlen Aug 15, 2023 •

edited

Loading

syan095 Aug 15, 2023

dandanlen Aug 14, 2023

syan095 Aug 15, 2023

dandanlen Aug 15, 2023 •

edited

Loading

syan095 Aug 15, 2023

dandanlen left a comment

feat: RPC call for amount_to_liquidity #3835

feat: RPC call for amount_to_liquidity #3835

Conversation

syan095 commented Aug 14, 2023 • edited Loading

Pull Request

Checklist

Summary

linear bot commented Aug 14, 2023

codecov bot commented Aug 14, 2023 • edited Loading

Codecov Report

dandanlen Aug 14, 2023

Choose a reason for hiding this comment

syan095 Aug 15, 2023

Choose a reason for hiding this comment

dandanlen Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

syan095 Aug 15, 2023

Choose a reason for hiding this comment

dandanlen Aug 14, 2023

Choose a reason for hiding this comment

syan095 Aug 15, 2023

Choose a reason for hiding this comment

dandanlen Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

syan095 Aug 15, 2023

Choose a reason for hiding this comment

dandanlen left a comment

Choose a reason for hiding this comment

syan095 commented Aug 14, 2023 •

edited

Loading

codecov bot commented Aug 14, 2023 •

edited

Loading

dandanlen Aug 15, 2023 •

edited

Loading

dandanlen Aug 15, 2023 •

edited

Loading