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(custom-rpc): add broker info [WEB-925] #4560

Merged
merged 3 commits into from
Feb 26, 2024
Merged

feat(custom-rpc): add broker info [WEB-925] #4560

merged 3 commits into from
Feb 26, 2024

Conversation

acdibble
Copy link
Contributor

Pull Request

Closes: WEB-925

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

Please include a succinct description of the purpose and content of the PR. What problem does it solve, and how? Link issues, discussions, other PRs, and anything else that will help the reviewer.

Non-Breaking changes

If this PR includes non-breaking changes, select the non-breaking label. On merge, CI will automatically cherry-pick the commit to a PR against the release branch.

@acdibble acdibble requested a review from dandanlen as a code owner February 23, 2024 09:16
Comment on lines 315 to 320
#[method(name = "account_info_v2")]
fn cf_account_info_v2(
&self,
account_id: state_chain_runtime::AccountId,
at: Option<state_chain_runtime::Hash>,
) -> RpcResult<RpcAccountInfoV2>;
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's a breaking change but it's a good time to do it with 1.3 coming soon and this old one being deprecated and only used for getting validator info

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

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

Project coverage is 72%. Comparing base (38f4443) to head (a251b24).
Report is 3 commits behind head on main.

Files Patch % Lines
state-chain/runtime/src/lib.rs 0% 0 Missing and 15 partials ⚠️
state-chain/custom-rpc/src/lib.rs 81% 6 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #4560    +/-   ##
======================================
- Coverage     73%     72%    -0%     
======================================
  Files        401     401            
  Lines      66210   66090   -120     
  Branches   66210   66090   -120     
======================================
- Hits       48089   47897   -192     
- Misses     15802   15866    +64     
- Partials    2319    2327     +8     

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

Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

Tagging @AlastairHolmes for visibility.

@@ -1070,7 +1070,7 @@ impl_runtime_apis! {
fn cf_account_flip_balance(account_id: &AccountId) -> u128 {
pallet_cf_flip::Account::<Runtime>::get(account_id).total()
}
fn cf_account_info_v2(account_id: &AccountId) -> RuntimeApiAccountInfoV2 {
fn cf_validator_info(account_id: &AccountId) -> RuntimeApiAccountInfoV2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If RuntimeApiAccountInfoV2 is now only used for validators, we could rename it as ValidatorInfo to be consistent with other naming.

As a next step (I'll open issue) we could factor out duplicate information and have a struct like AccountInfo<Role> where the role defines the additional information for each role. Something like this:

struct AccountInfo<RoleInfo> {
	balance: NumberOrHex,
	#[serde(flatten)]
	role_info: RoleInfo,
}

enum RpcAccountInfo {
	Unregistered(AccountInfo<()>),
	Lp(AccountInfo<LpAccountInfo>),
	//..
}

It should be possible to do this without chaning the encoding, and it will make things easier to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do, sounds good!

@AlastairHolmes
Copy link
Contributor

AlastairHolmes commented Feb 23, 2024

We sure that rpc isn't being used, did we ask?

@acdibble
Copy link
Contributor Author

We sure that rpc isn't being used, did we ask?

No, but all the information is available in account_info, I'll ask around

@acdibble
Copy link
Contributor Author

I've reverted the breaking change because I don't think anyone is using this because it's not documented and we haven't encouraged anyone to use it, but they might be and I don't feel like figuring it out. It's more important to get the additions in now.

@acdibble acdibble merged commit 307485c into main Feb 26, 2024
43 checks passed
@acdibble acdibble deleted the feat/broker-info branch February 26, 2024 15:43
syan095 added a commit that referenced this pull request Feb 26, 2024
…ero-liquidity

* origin/main: (28 commits)
  feat(custom-rpc): add broker info [WEB-925] (#4560)
  chore: upgrade solana version (#4567)
  fix: continuous adapter (PRO-684) (#4503)
  fix: Wait for ThresholdSignature success before switching to NewKeysActivated (#4534)
  chore: remove un-needed serde derives from EncodedAddress (#4565)
  fix: more lenient max deposit fee in bouncer test (#4564)
  chore: build persa bins instead of fetch (#4554)
  feat: deploy L2 contracts upon localnet startup and send L2 TXs 📑 (#4558)
  feat: debug logs on runtime upgrade test (#4556)
  feature/PRO-1038/pool-fee-rpc (#4459)
  chore: update bootnodes in chainspec ✨ (#4456)
  fix: allow test coverage to run (#4555)
  refactor/pro-1160/remove-side-side-map (#4489)
  fix: activate missing migrations (#4550)
  chore: remove cf-build (#4551)
  feat: extensible multi-key rotator (#4546)
  fix: ensure channel open fee can be paid in benchmarks (#4544)
  feat: bump substrate deps to polkadot-sdk 1.6 (#4504)
  fix: upgrade-test should restart the chainflip-nodes on an incompatible upgrade (#4490)
  test: add small v3 migration test (#4543)
  ...

# Conflicts:
#	state-chain/pallets/cf-pools/src/tests.rs
syan095 added a commit that referenced this pull request Feb 26, 2024
…-price

* origin/main:
  feat(custom-rpc): add broker info [WEB-925] (#4560)
  chore: upgrade solana version (#4567)
  fix: continuous adapter (PRO-684) (#4503)
  fix: Wait for ThresholdSignature success before switching to NewKeysActivated (#4534)
  chore: remove un-needed serde derives from EncodedAddress (#4565)
  fix: more lenient max deposit fee in bouncer test (#4564)
  chore: build persa bins instead of fetch (#4554)
  feat: deploy L2 contracts upon localnet startup and send L2 TXs 📑 (#4558)
  feat: debug logs on runtime upgrade test (#4556)
  feature/PRO-1038/pool-fee-rpc (#4459)
  chore: update bootnodes in chainspec ✨ (#4456)
  fix: allow test coverage to run (#4555)
  refactor/pro-1160/remove-side-side-map (#4489)
  fix: activate missing migrations (#4550)
  chore: remove cf-build (#4551)

# Conflicts:
#	state-chain/pallets/cf-pools/src/lib.rs
syan095 added a commit that referenced this pull request Feb 27, 2024
…utxo

* origin/main:
  feat(custom-rpc): add broker info [WEB-925] (#4560)
  chore: upgrade solana version (#4567)
  fix: continuous adapter (PRO-684) (#4503)
  fix: Wait for ThresholdSignature success before switching to NewKeysActivated (#4534)
  chore: remove un-needed serde derives from EncodedAddress (#4565)
  fix: more lenient max deposit fee in bouncer test (#4564)
  chore: build persa bins instead of fetch (#4554)
  feat: deploy L2 contracts upon localnet startup and send L2 TXs 📑 (#4558)
  feat: debug logs on runtime upgrade test (#4556)
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.

3 participants