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: returning detailed account information for affiliate in the cf_account_info rpc for brokers #5641

Open
wants to merge 2 commits into
base: release/1.8
Choose a base branch
from

Conversation

Janislav
Copy link
Contributor

@Janislav Janislav commented Feb 14, 2025

Pull Request

Closes: PRO-2038

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have written sufficient tests.
  • I have written and tested required migrations.
  • I have updated documentation where appropriate.

Summary

  • Return affiliate details + usdc balance of affiliates for a broker account
  • Deprecated earned_fees field

@Janislav Janislav requested a review from dandanlen as a code owner February 14, 2025 10:35
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
state-chain/custom-rpc/src/lib.rs 86% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@dandanlen
Copy link
Collaborator

I think the idea was to also add account balances for all account types, so that it would also show if the account is an unregistered affiliate.

Something like this:

pub struct RpcAccountInfoWithBalances {
	asset_balances: any::AssetMap<NumberOrHex>,
	#[serde(flatten)]
	account_info: RpcAccountInfo,
}

And then change the rpc return type to RpcAccountInfoWithBalances.

@@ -240,9 +247,11 @@ pub enum RpcAccountInfo {
Broker {
flip_balance: NumberOrHex,
bond: NumberOrHex,
#[deprecated(note = "This field is deprecated and will be replaced in a future release")]
earned_fees: any::AssetMap<NumberOrHex>,
affiliates: Vec<(AffiliateShortId, AccountId)>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might as well deprecate this too and add the account_id into RpcAffiliateDetails.

pub struct RpcAffiliateDetails {
pub short_id: AffiliateShortId,
pub withdrawal_address: EthereumAddress,
pub earned_fees: NumberOrHex,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub earned_fees: NumberOrHex,
pub balance: NumberOrHex,

Feedback was that earned_fees was misleading because it didn't include 'all time' fees.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think balance is misleading since it is not the on chain balance but the collected fees in usdc

@@ -197,6 +197,7 @@ pub struct BrokerInfo {
pub btc_vault_deposit_address: Option<String>,
pub affiliates: Vec<(AffiliateShortId, AccountId32)>,
pub bond: AssetAmount,
pub affiliate_details: Vec<(AffiliateDetails, AssetAmount)>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately this is a breaking change to the runtime API. We can do this but we should manage it using runtime api versioning (see the doc comment on the CustomRuntimeApi trait).

Alternatively maybe we can get the data we need via get_affiliates.

@Janislav Janislav force-pushed the feat/pro-2038/improved-support-for-affiliates branch from 1d1ec89 to 4301661 Compare February 14, 2025 14:35
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