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.

Sign up for GitHub

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

Jump to bottom

feat: cf_boost_pool_details rpc #4780

Merged
merged 8 commits into from
Apr 26, 2024
Merged

feat: cf_boost_pool_details rpc #4780

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2d6b9b2
feat: cf_boost_pool_details rpc
msgmaxim Apr 19, 2024
a7f6537
Merge branch 'main' into feat/boost-details-rpc
msgmaxim Apr 23, 2024
0f348c0
feat: update boost rpc schema
msgmaxim Apr 24, 2024
8fe9460
feat: maps -> lists in boost rpc; amounts as hex
msgmaxim Apr 25, 2024
13bd48d
feat: optional asset param in boost deatils rpc
msgmaxim Apr 25, 2024
716a4c9
feat: flatten asset in boost rpc response
msgmaxim Apr 25, 2024
b2bec41
Merge branch 'main' into feat/boost-details-rpc
msgmaxim Apr 26, 2024
b41969d
fix: derive serde attributes conditionally
msgmaxim Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions state-chain/custom-rpc/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ use state_chain_runtime::{
chainflip::{BlockUpdate, Offence},
constants::common::TX_FEE_MULTIPLIER,
runtime_apis::{
BoostPoolDepth, BrokerInfo, CustomRuntimeApi, DispatchErrorWithMessage, EventFilter,
FailingWitnessValidators, LiquidityProviderInfo, ValidatorInfo,
BoostPoolDepth, BoostPoolDetails, BrokerInfo, CustomRuntimeApi, DispatchErrorWithMessage,
EventFilter, FailingWitnessValidators, LiquidityProviderInfo, ValidatorInfo,
},
NetworkFee,
};
Expand Down Expand Up @@ -562,6 +562,9 @@ pub trait CustomApi {

#[method(name = "boost_pools_depth")]
fn cf_boost_pools_depth(&self) -> RpcResult<Vec<BoostPoolDepth>>;

#[method(name = "boost_pool_details")]
fn cf_boost_pool_details(&self, asset: Asset) -> RpcResult<BTreeMap<u16, BoostPoolDetails>>;
}

/// An RPC extension for the state chain node.
Expand Down Expand Up @@ -1380,6 +1383,13 @@ where
.map(|event| event.encode().into())
.collect::<Vec<_>>())
}

fn cf_boost_pool_details(&self, asset: Asset) -> RpcResult<BTreeMap<u16, BoostPoolDetails>> {
self.client
.runtime_api()
.cf_boost_pool_details(self.client.info().best_hash, asset)
.map_err(to_rpc_error)
}
}

impl<C, B> CustomRpc<C, B>
Expand Down
33 changes: 32 additions & 1 deletion state-chain/pallets/cf-ingress-egress/src/boost_pool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,37 @@ where
self.available_amount.into_chain_amount()
}

pub fn get_amounts(&self) -> BTreeMap<AccountId, C::ChainAmount> {
self.amounts
.iter()
.map(|(account_id, scaled_amount)| {
(account_id.clone(), scaled_amount.into_chain_amount())
})
.collect()
}

pub fn get_pending_boosts(
&self,
) -> BTreeMap<PrewitnessedDepositId, BTreeMap<AccountId, C::ChainAmount>> {
self.pending_boosts
.iter()
.map(|(deposit_id, owed_amounts_map)| {
(
*deposit_id,
owed_amounts_map
.iter()
.map(|(account_id, scaled_amount)| {
(account_id.clone(), scaled_amount.into_chain_amount())
})
.collect(),
)
})
.collect()
}

pub fn get_pending_withdrawals(&self) -> &BTreeMap<AccountId, BTreeSet<PrewitnessedDepositId>> {
&self.pending_withdrawals
}
pub(crate) fn provide_funds_for_boosting(
&mut self,
prewitnessed_deposit_id: PrewitnessedDepositId,
Expand Down Expand Up @@ -340,7 +371,7 @@ where
}

#[cfg(test)]
pub fn get_pending_boosts(&self) -> Vec<PrewitnessedDepositId> {
pub fn get_pending_boost_ids(&self) -> Vec<PrewitnessedDepositId> {
self.pending_boosts.keys().copied().collect()
}
#[cfg(test)]
Expand Down
23 changes: 6 additions & 17 deletions state-chain/pallets/cf-ingress-egress/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ use cf_chains::{
FetchAssetParams, ForeignChainAddress, SwapOrigin, TransferAssetParams,
};
use cf_primitives::{
Asset, BasisPoints, BroadcastId, ChannelId, EgressCounter, EgressId, EpochIndex, ForeignChain,
PrewitnessedDepositId, SwapId, ThresholdSignatureRequestId,
Asset, BasisPoints, BoostPoolTier, BroadcastId, ChannelId, EgressCounter, EgressId, EpochIndex,
ForeignChain, PrewitnessedDepositId, SwapId, ThresholdSignatureRequestId,
};
use cf_runtime_utilities::log_or_panic;
use cf_traits::{
Expand All @@ -53,7 +53,6 @@ use sp_std::{
vec,
vec::Vec,
};
use strum_macros::EnumIter;
pub use weights::WeightInfo;

#[derive(Clone, Debug, PartialEq, Eq, Encode, Decode, TypeInfo)]
Expand All @@ -71,28 +70,18 @@ pub struct PrewitnessedDeposit<C: Chain> {
pub deposit_details: C::DepositDetails,
}

// TODO: use u16 directly so we can dynamically add/remove pools?
#[derive(
Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Encode, Decode, TypeInfo, EnumIter,
)]
#[repr(u16)]
pub enum BoostPoolTier {
FiveBps = 5,
TenBps = 10,
ThirtyBps = 30,
}

#[derive(Clone, Debug, PartialEq, Eq, Encode, Decode, TypeInfo)]
pub struct BoostPoolId<C: Chain> {
asset: C::ChainAsset,
tier: BoostPoolTier,
}

pub struct BoostOutput<C: Chain> {
used_pools: BTreeMap<BoostPoolTier, C::ChainAmount>,
total_fee: C::ChainAmount,
}

const SORTED_BOOST_TIERS: [BoostPoolTier; 3] =
pub const SORTED_BOOST_TIERS: [BoostPoolTier; 3] =
[BoostPoolTier::FiveBps, BoostPoolTier::TenBps, BoostPoolTier::ThirtyBps];

/// Enum wrapper for fetch and egress requests.
Expand Down Expand Up @@ -202,7 +191,7 @@ pub mod pallet {
pub(crate) type ChannelRecycleQueue<T, I> =
Vec<(TargetChainBlockNumber<T, I>, TargetChainAccount<T, I>)>;

pub(crate) type TargetChainAsset<T, I> = <<T as Config<I>>::TargetChain as Chain>::ChainAsset;
pub type TargetChainAsset<T, I> = <<T as Config<I>>::TargetChain as Chain>::ChainAsset;
pub(crate) type TargetChainAccount<T, I> =
<<T as Config<I>>::TargetChain as Chain>::ChainAccount;
pub(crate) type TargetChainAmount<T, I> = <<T as Config<I>>::TargetChain as Chain>::ChainAmount;
Expand Down Expand Up @@ -349,7 +338,7 @@ pub mod pallet {
use strum::IntoEnumIterator;

for asset in TargetChainAsset::<T, I>::iter() {
for pool_tier in BoostPoolTier::iter() {
for pool_tier in SORTED_BOOST_TIERS {
BoostPools::<T, I>::set(
asset,
pool_tier,
Expand Down
2 changes: 1 addition & 1 deletion state-chain/pallets/cf-ingress-egress/src/migrations/add_boost_pools.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl<T: Config<I>, I: 'static> OnRuntimeUpgrade for Migration<T, I> {
// Create boost pools:
use strum::IntoEnumIterator;
for asset in TargetChainAsset::<T, I>::iter() {
for pool_tier in BoostPoolTier::iter() {
for pool_tier in SORTED_BOOST_TIERS {
BoostPools::<T, I>::set(
asset,
pool_tier,
Expand Down
4 changes: 2 additions & 2 deletions state-chain/pallets/cf-ingress-egress/src/tests/boost.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,7 @@ fn lost_funds_are_acknowledged_by_boost_pool() {
assert_eq!(
BoostPools::<Test>::get(eth::Asset::Eth, BoostPoolTier::FiveBps)
.unwrap()
.get_pending_boosts(),
.get_pending_boost_ids(),
vec![deposit_id]
);

Expand All @@ -741,7 +741,7 @@ fn lost_funds_are_acknowledged_by_boost_pool() {

assert!(BoostPools::<Test>::get(eth::Asset::Eth, BoostPoolTier::FiveBps)
.unwrap()
.get_pending_boosts()
.get_pending_boost_ids()
.is_empty());
}
});
Expand Down
9 changes: 9 additions & 0 deletions state-chain/primitives/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,15 @@ pub type SwapId = u64;

pub type PrewitnessedDepositId = u64;

// TODO: use u16 directly so we can dynamically add/remove pools?
#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Encode, Decode, TypeInfo)]
#[repr(u16)]
pub enum BoostPoolTier {
FiveBps = 5,
TenBps = 10,
ThirtyBps = 30,
}

/// The type of the Id given to threshold signature requests. Note a single request may
/// result in multiple ceremonies, but only one ceremony should succeed.
pub type ThresholdSignatureRequestId = u32;
Expand Down
42 changes: 39 additions & 3 deletions state-chain/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ mod weights;
use crate::{
chainflip::{calculate_account_apy, Offence},
runtime_apis::{
AuctionState, BoostPoolDepth, BrokerInfo, DispatchErrorWithMessage, EventFilter,
FailingWitnessValidators, LiquidityProviderInfo, RuntimeApiPenalty, ValidatorInfo,
AuctionState, BoostPoolDepth, BoostPoolDetails, BrokerInfo, DispatchErrorWithMessage,
EventFilter, FailingWitnessValidators, LiquidityProviderInfo, RuntimeApiPenalty,
ValidatorInfo,
},
};
use cf_amm::{
Expand All @@ -38,7 +39,7 @@ use core::ops::Range;
use frame_support::instances::*;
pub use frame_system::Call as SystemCall;
use pallet_cf_governance::GovCallHash;
use pallet_cf_ingress_egress::{ChannelAction, DepositWitness};
use pallet_cf_ingress_egress::{ChannelAction, DepositWitness, TargetChainAsset};
use pallet_cf_pools::{
AskBidMap, AssetPair, PoolLiquidity, PoolOrderbook, PoolPriceV1, PoolPriceV2,
UnidirectionalPoolDepth,
Expand Down Expand Up @@ -1642,6 +1643,41 @@ impl_runtime_apis! {
}).collect()

}

fn cf_boost_pool_details(asset: Asset) -> BTreeMap<u16, BoostPoolDetails> {

fn boost_pools_details<I: 'static>(asset: TargetChainAsset::<Runtime, I>) -> BTreeMap<u16, BoostPoolDetails>
where Runtime: pallet_cf_ingress_egress::Config<I> {

pallet_cf_ingress_egress::BoostPools::<Runtime, I>::iter_prefix(asset).map(|(tier, pool)| {
(
tier as u16,
BoostPoolDetails {
available_amount: pool.get_available_amount().into(),
amounts: pool.get_amounts().into_iter().map(|(id, amount)| (id, amount.into())).collect(),
pending_boosts: pool.get_pending_boosts().into_iter().map(|(deposit_id, owed_amounts)| {
(
deposit_id,
owed_amounts.into_iter().map(|(id, amount)| (id, amount.into())).collect()
)
}).collect(),
pending_withdrawals: pool.get_pending_withdrawals().clone(),
}
)
}).collect()

}

let chain: ForeignChain = asset.into();

match chain {
ForeignChain::Ethereum => boost_pools_details::<EthereumInstance>(asset.try_into().unwrap()),
ForeignChain::Polkadot => boost_pools_details::<PolkadotInstance>(asset.try_into().unwrap()),
ForeignChain::Bitcoin => boost_pools_details::<BitcoinInstance>(asset.try_into().unwrap()),
ForeignChain::Arbitrum => boost_pools_details::<ArbitrumInstance>(asset.try_into().unwrap()),
}

}
}

// END custom runtime APIs
Expand Down
16 changes: 14 additions & 2 deletions state-chain/runtime/src/runtime_apis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use cf_chains::{
};
use cf_primitives::{
AccountRole, Asset, AssetAmount, BlockNumber, BroadcastId, EpochIndex, FlipBalance,
ForeignChain, NetworkEnvironment, SemVer, SwapOutput,
ForeignChain, NetworkEnvironment, PrewitnessedDepositId, SemVer, SwapOutput,
};
use codec::{Decode, Encode};
use core::ops::Range;
Expand All @@ -25,7 +25,10 @@ use scale_info::{prelude::string::String, TypeInfo};
use serde::{Deserialize, Serialize};
use sp_api::decl_runtime_apis;
use sp_runtime::DispatchError;
use sp_std::{collections::btree_map::BTreeMap, vec::Vec};
use sp_std::{
collections::{btree_map::BTreeMap, btree_set::BTreeSet},
vec::Vec,
};

type VanityName = Vec<u8>;

Expand Down Expand Up @@ -66,6 +69,14 @@ pub struct BoostPoolDepth {
pub available_amount: AssetAmount,
}

#[derive(Encode, Decode, Eq, PartialEq, TypeInfo, Serialize, Deserialize)]
pub struct BoostPoolDetails {
pub available_amount: AssetAmount,
pub amounts: BTreeMap<AccountId32, AssetAmount>,
GabrielBuragev marked this conversation as resolved.
Show resolved Hide resolved
pub pending_boosts: BTreeMap<PrewitnessedDepositId, BTreeMap<AccountId32, AssetAmount>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@GabrielBuragev GabrielBuragev Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@msgmaxim msgmaxim Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@GabrielBuragev GabrielBuragev Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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!

pub pending_withdrawals: BTreeMap<AccountId32, BTreeSet<PrewitnessedDepositId>>,
}

#[derive(Encode, Decode, Eq, PartialEq, TypeInfo)]
pub struct RuntimeApiPenalty {
pub reputation_points: i32,
Expand Down Expand Up @@ -229,5 +240,6 @@ decl_runtime_apis!(
fn cf_channel_opening_fee(chain: ForeignChain) -> FlipBalance;
fn cf_get_events(filter: EventFilter) -> Vec<EventRecord<RuntimeEvent, Hash>>;
fn cf_boost_pools_depth() -> Vec<BoostPoolDepth>;
fn cf_boost_pool_details(asset: Asset) -> BTreeMap<u16, BoostPoolDetails>;
}
);
Loading