feat: cf_boost_pool_details rpc #4780

msgmaxim · 2024-04-19T07:33:12Z

Pull Request

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 cf_boost_pool_details rpc call which takes asset as the sole parameter and returns details (amounts, pendings boosts and pending withdrawals) of all existing pools for that asset. I wasn't sure if we wanted pool tier as a parameter as well (see my question in PRO-1324), but the approach I took here made the most sense to me (changing this wouldn't be difficult, so happy to do so if necessary).

I considered simply making the fields of BoostPool public and read them directly when constructing BoostPoolDetails in the rpc handler, but in the end I thought it was important to keep ScaledAmount an implementation detail of BoostPool, so I added getters which convert all usages of ScaledAmount to ChainAsset first (which is consitent with the rest of BoostPool's interface.)

Usage:

curl -H "Content-Type: application/json" -d '{"id":1, "jsonrpc":"2.0", "method": "cf_boost_pool_details", "params": ["ETH", "10"]}' http://localhost:9944

See sample response here: https://github.com/chainflip-io/chainflip-backend/blob/716a4c9e0bcfb85ea3f3bfac6d8304a87457eed7/state-chain/custom-rpc/src/snapshots/custom_rpc__test__boost_details_serialization.snap

codecov · 2024-04-19T07:40:36Z

Codecov Report

Attention: Patch coverage is 51.08696% with 90 lines in your changes are missing coverage. Please review.

Project coverage is 72%. Comparing base (13647ee) to head (b41969d).

Files	Patch %	Lines
state-chain/runtime/src/lib.rs	0%	0 Missing and 34 partials ⚠️
...-chain/pallets/cf-ingress-egress/src/boost_pool.rs	3%	29 Missing ⚠️
state-chain/custom-rpc/src/lib.rs	77%	24 Missing ⚠️
state-chain/runtime/src/runtime_apis.rs	82%	0 Missing and 2 partials ⚠️
state-chain/primitives/src/lib.rs	75%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #4780    +/-   ##
======================================
- Coverage     72%     72%    -0%     
======================================
  Files        412     412            
  Lines      70494   70526    +32     
  Branches   70494   70526    +32     
======================================
- Hits       51012   50902   -110     
- Misses     17022   17128   +106     
- Partials    2460    2496    +36

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

kylezs

Looks good to me, would await @GabrielBuragev 's approval before merging

GabrielBuragev · 2024-04-23T07:33:42Z

Are the parameters (asset, tier) optional?
I think its important to be able to call this endpoint without any parameters and it would return the details for all pools so we cut down on requests.

Also, check the preferred spec for the response schema here: https://www.notion.so/chainflip/BOOST-aa8375e7bfa14c0895bf234047a0a972?pvs=4#5ed657173cdc4d8a81840ca8734bd316

state-chain/runtime/src/runtime_apis.rs

GabrielBuragev · 2024-04-23T08:28:04Z

state-chain/runtime/src/runtime_apis.rs

+pub struct BoostPoolDetails {
+	pub available_amount: AssetAmount,
+	pub amounts: BTreeMap<AccountId32, AssetAmount>,
+	pub pending_boosts: BTreeMap<PrewitnessedDepositId, BTreeMap<AccountId32, AssetAmount>>,


pending_boosts might be a bit confusing here. Maybe deposits_pending_finalization or pending_amounts ?

Also, can we adjust it so it follows the specs defined?

deposits_pending_finalization: [{ channel_id, source_chain, asset, booster_shares:[{ account_id, amount }] }] // We can derive the total in_use amount from this

Not sure how I missed this part of the spec on Notion (I find Notion updates hard to track sometimes...). I thought I'd just start by doing the easiest thing, which is exposing all information (which is more or less how it is described in the linear issue), and make any adjustments if requested.

Now that I see the spec, I have questions:

Why is it necessary to include source_chain/asset? This RPC for a specific asset already, so it is already known by the caller of this RPC.

Do we actually want channel_id rather than the deposit id (as I'm providing currently)? Channels can have more than one deposit, so I think the deposit id might be necessary to uniquely identify the deposit, and it is more the question whether we want to additionally provide channel_id.

Another question is regarding pending_withdrawal_amount from the schema:

pending_withdrawal_amount: [{ account_id, amount }],

Does this mean that we are only interested in the amounts (the sum of them?) rather than which deposits we are waiting for? Seems like both are important and if you have the deposit id (as I provide currently), you'd be able to derive the amount, but not the other way around.

Why is it necessary to include source_chain/asset? This RPC for a specific asset already, so it is already known by the caller of this RPC.

Initially, i thought this RPC would have the ability to return the details for all the existing boost pools and the asset/tier params would be optional. The preferred way would be to allow the following:

No params provided - return the details for all boost pools

asset param provided - return the details for all boost pools for the provided asset

In the first case we will need the asset/source_chain included in the specific object. So lets include it and enable these 2 use cases above.

Do we actually want channel_id rather than the deposit id (as I'm providing currently)? Channels can have more than one deposit, so I think the deposit id might be necessary to uniquely identify the deposit, and it is more the question whether we want to additionally provide channel_id.

Thats a good point. Lets include the prewitnessed_deposit_id here. But lets also keep the other fields for convenience please (channel_id, source_chain, asset). We can do a clean up in the end based on what ended up being used or not. I really can not recall the exact reason why we included channel_id here and not prewitnessed_deposit_id.

Does this mean that we are only interested in the amounts (the sum of them?) rather than which deposits we are waiting for? Seems like both are important and if you have the deposit id (as I provide currently), you'd be able to derive the amount, but not the other way around.

Its a good point, we might further improve this rpc by including the deposit ids besides the other fields too (account_id, amount).
I would rather we expose as much details as possible on this RPC. Exposing only the ids means that we have to look elsewhere to get the details related to that deposit.

Exposing only the ids means that we have to look elsewhere to get the details related to that deposit.

But the amounts are already exposed this very response under pending_boosts. I don't think we should return the amounts twice in the same response (similar to how you suggested that total_available_amount wasn't necessary).

But lets also keep the other fields for convenience please (channel_id, source_chain, asset). We can do a clean up in the end based on what ended up being used or not. I really can not recall the exact reason why we included channel_id here and not prewitnessed_deposit_id.

There is no problem adding source_chain and asset, but the reason I asked about channel_id specifically is that we don't currently store that information in the boost pool. Let's think whether it is actually required (or might be required in the future), and if so, think of the best way to provide it.

Exposing only the ids means that we have to look elsewhere to get the details related to that deposit.

But the amounts are already exposed this very response under pending_boosts. I don't think we should return the amounts twice in the same response (similar to how you suggested that total_available_amount wasn't necessary).

You are right. After seeing the snap i have a better understanding of your comment. The current schema looks good!

GabrielBuragev · 2024-04-23T08:37:24Z

One more thing, how hard would it be to allow filtering the results by account_id ?
Its a part of the spec but not sure how huge of a lift that is.

GabrielBuragev · 2024-04-23T08:50:22Z

"result": {
"5": {
"available_amount": 1000,
"amounts": {
"cFPdef3hF5zEwbWUG6ZaCJ3X7mTvEeAog7HxZ8QyFcCgDVGDM": 1000
},
"pending_boosts": {},
"pending_withdrawals": {}
},
"10": {
"available_amount": 0,
"amounts": {},
"pending_boosts": {
"1": {
"cFHsUq1uK5opJudRDczhdPVj6LGoVTqYsfj71tbHfKsTAzkJJ": 500
}
},
"pending_withdrawals": {
"cFHsUq1uK5opJudRDczhdPVj6LGoVTqYsfj71tbHfKsTAzkJJ": [
1
]
}
},
"30": {
"available_amount": 0,
"amounts": {},
"pending_boosts": {},
"pending_withdrawals": {}
},
},

Can we return an array instead of an object with feeTier keys? The defined schema spec should give you an idea of the preferred format for the response.

msgmaxim · 2024-04-24T03:03:06Z

One more thing, how hard would it be to allow filtering the results by account_id ? Its a part of the spec but not sure how huge of a lift that is.

Can I ask how this will be used? I imagined that if we provide all the details from all pools, the web app could periodically request it, cache it somewhere, and then derive any information it needs to display, and most of the time it will want everything anyway. Or are you planning to make an RPC call every time a user requests it (i.e. without caching)?

It wouldn't be difficult to allow filtering by account_id, but it feels like if something can (as easily) be done in in the app, it should be done there?

GabrielBuragev · 2024-04-24T14:51:06Z

Can I ask how this will be used?

It will be used differently in different places.

On the LP app, we will probably use it on the client side (web) to show the stats for the specific LP
In the processor/indexer we will have to use this RPC (specifically the deposits_pending_finalization) to associate the shares per account for each boost (this was decided as an alternative to emitting these amounts in an event which would be a lot of data)
In our cache, we will probably use this to keep in sync with the current state chain state so we can use it in the block explorer and other apps (displaying non-historical data, only present)

Lets ignore the filtering by account_id for now and include it at a later point if really needed. Its not a hard requirement.

msgmaxim · 2024-04-25T01:25:55Z

Lets ignore the filtering by account_id for now and include it at a later point if really needed. Its not a hard requirement.

OK. Though I still want to understand the motivation behind this. If there is a good reason, we should provide this.

On the LP app, we will probably use it on the client side (web) to show the stats for the specific LP

Makes sense if it will be used by clients' browsers. Though my understanding was that node's RPC server was to be used by the app's backend, and the users' browsers aren't making direct rpc requrests to a node. If the calls go through the backend, then you'd already have all the data for each LP on the backend, and you could filter internally. Is there a reason for clients to call node RPCs directly (other than this being potentially more work for the backend to expose this)?

In the processor/indexer we will have to use this RPC (specifically the deposits_pending_finalization) to associate the shares per account for each boost

In our cache, we will probably use this to keep in sync

It seems like for these two the filtering by LP isn't needed? I was mainly asking for when we'd need to filter by account_id (though I appreciate your comprehensive response).

msgmaxim · 2024-04-25T07:50:42Z

Updates:

Included insta tests (I will now close test: snapshot testing for new rpcs #4792). You can now see the snap file to check how the resonse is formatted.
Asset is now an optional parameter; if not specificed, pools for all assets will be returned. For this I have added chain/asset into every boost details entry.
All maps are now lists with the "key" as a extra field (to match the schema in notion)
All amounts are now encoded as hex (updated the boost_depth rpc too)
It seemed strange to see chain/asset nested inside another "asset" field, so I instead flattened it for both boost rpcs (see the snap file); happy to revert if the nesting was somehow desirable.

@GabrielBuragev @kylezs

GabrielBuragev · 2024-04-25T08:59:00Z

Makes sense if it will be used by clients' browsers.

To give you a bit more context on how the LP app was structured. We ended up doing some of the RPC calls directly on the client side (LP app) back in the days so we could ship it out faster and in time (without having to setup a whole backend for it). Having that in mind, i thought it might be useful to have that filtering baked in so we could utilize it there without any extra effort. But when i think about it deeper now, we anyways want to start moving most of the stuff on the LP app to its own backend, so this might be a good time to start that. We will still be able to utilize the cache and filter by account ids there.

We can move forward without that. Thanks for pointing out these things, made me re-think :D

* origin/main: Arbitrum gas limit multiplier (#4781) fix: don't set code red on "agg-key set by gov-key" (#4813) fix: sign tx with correct key during rotation (#4794) chore: arbitrum witnesser permission (#4798) Solana (#4414) feat: try-runtime build step on dev ci (#4807) feat: optimistic build, streamlined ci-main (#4806) chore: log raw dispatch error (#4809) feat: allow for single binary CFE upgrades (#4634) fix: take fee on to usdc (#4801) (#4804) refactor: minor cleanup of retrier code and vault pallet (#4803) feat: cf_boost_pool_details rpc (#4780) # Conflicts: # Cargo.lock # state-chain/chains/src/sol.rs # state-chain/chains/src/sol/program_instructions.rs # state-chain/chains/src/sol/sol_tx_building_blocks.rs # state-chain/chains/src/sol/token_instructions.rs # state-chain/runtime/src/safe_mode.rs

feat: cf_boost_pool_details rpc

2d6b9b2

msgmaxim requested review from kylezs and GabrielBuragev April 19, 2024 07:33

msgmaxim requested a review from dandanlen as a code owner April 19, 2024 07:33

kylezs approved these changes Apr 22, 2024

View reviewed changes

j4m1ef0rd approved these changes Apr 23, 2024

View reviewed changes

Merge branch 'main' into feat/boost-details-rpc

a7f6537

GabrielBuragev reviewed Apr 23, 2024

View reviewed changes

feat: update boost rpc schema

0f348c0

msgmaxim added 3 commits April 25, 2024 15:23

feat: maps -> lists in boost rpc; amounts as hex

8fe9460

feat: optional asset param in boost deatils rpc

13bd48d

feat: flatten asset in boost rpc response

716a4c9

GabrielBuragev approved these changes Apr 25, 2024

View reviewed changes

msgmaxim mentioned this pull request Apr 25, 2024

test: snapshot testing for new rpcs #4792

Closed

2 tasks

kylezs enabled auto-merge April 25, 2024 13:28

msgmaxim added 2 commits April 26, 2024 11:29

Merge branch 'main' into feat/boost-details-rpc

b2bec41

fix: derive serde attributes conditionally

b41969d

msgmaxim force-pushed the feat/boost-details-rpc branch from d1450fd to b41969d Compare April 26, 2024 04:18

kylezs added this pull request to the merge queue Apr 26, 2024

Merged via the queue into main with commit 68a5d74 Apr 26, 2024
43 checks passed

kylezs deleted the feat/boost-details-rpc branch April 26, 2024 05:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cf_boost_pool_details rpc #4780

feat: cf_boost_pool_details rpc #4780

msgmaxim commented Apr 19, 2024 •

edited by kylezs

Loading

codecov bot commented Apr 19, 2024 •

edited

Loading

kylezs left a comment

GabrielBuragev commented Apr 23, 2024 •

edited

Loading

GabrielBuragev Apr 23, 2024

GabrielBuragev Apr 23, 2024

msgmaxim Apr 24, 2024

msgmaxim Apr 24, 2024

GabrielBuragev Apr 24, 2024 •

edited

Loading

GabrielBuragev Apr 24, 2024

msgmaxim Apr 25, 2024 •

edited

Loading

msgmaxim Apr 25, 2024

GabrielBuragev Apr 25, 2024 •

edited

Loading

GabrielBuragev commented Apr 23, 2024

GabrielBuragev commented Apr 23, 2024

msgmaxim commented Apr 24, 2024

GabrielBuragev commented Apr 24, 2024

msgmaxim commented Apr 25, 2024 •

edited

Loading

msgmaxim commented Apr 25, 2024

GabrielBuragev commented Apr 25, 2024 •

edited

Loading

feat: cf_boost_pool_details rpc #4780

feat: cf_boost_pool_details rpc #4780

Conversation

msgmaxim commented Apr 19, 2024 • edited by kylezs Loading

Pull Request

Checklist

Summary

codecov bot commented Apr 19, 2024 • edited Loading

Codecov Report

kylezs left a comment

Choose a reason for hiding this comment

GabrielBuragev commented Apr 23, 2024 • edited Loading

GabrielBuragev Apr 23, 2024

Choose a reason for hiding this comment

GabrielBuragev Apr 23, 2024

Choose a reason for hiding this comment

msgmaxim Apr 24, 2024

Choose a reason for hiding this comment

msgmaxim Apr 24, 2024

Choose a reason for hiding this comment

GabrielBuragev Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

GabrielBuragev Apr 24, 2024

Choose a reason for hiding this comment

msgmaxim Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

msgmaxim Apr 25, 2024

Choose a reason for hiding this comment

GabrielBuragev Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

GabrielBuragev commented Apr 23, 2024

GabrielBuragev commented Apr 23, 2024

msgmaxim commented Apr 24, 2024

GabrielBuragev commented Apr 24, 2024

msgmaxim commented Apr 25, 2024 • edited Loading

msgmaxim commented Apr 25, 2024

GabrielBuragev commented Apr 25, 2024 • edited Loading

msgmaxim commented Apr 19, 2024 •

edited by kylezs

Loading

codecov bot commented Apr 19, 2024 •

edited

Loading

GabrielBuragev commented Apr 23, 2024 •

edited

Loading

GabrielBuragev Apr 24, 2024 •

edited

Loading

msgmaxim Apr 25, 2024 •

edited

Loading

GabrielBuragev Apr 25, 2024 •

edited

Loading

msgmaxim commented Apr 25, 2024 •

edited

Loading

GabrielBuragev commented Apr 25, 2024 •

edited

Loading