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

refactor/improve-rpc-encode-decode #4737

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Janislav
Copy link
Contributor

@Janislav Janislav commented Apr 8, 2024

Pull Request

Closes: PRO-1157

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

  • Primarily removes any usage of NumberOrHex from the return types of the LP-API -> Should be not breaking because in any case LPs have to handle decoding the result as a hex string already.
  • Removes also all usage for NumberOrHex and any WrappedType that is using it in a V2 version of the API.
  • Provides a backwards compatible API to avoid breaking the LPs

Non-Breaking changes

In theory, that should be non-breaking. It still needs some more further checking to ensure that, though. We assume that because NumberOrHex can theoretically decode to a hex string, so LPs also have to handle that case already, which means replacing that with a type that always returns as hex should be none-breaking. Apart from that, during the transition over the next version, we keep the old API as it is and give LPs time to change their implementation. With an upcoming version, we will remove this old API.

Removed all usage of NumberOrHex from the LP inclusive all types that are relying on NumberOrHex. Removed all usage of NumberOrHex from the LP inclusive all types that are relying on NumberOrHex. Replaced it with a generic functions that takes a U256 and tries to parse it to the type of the generic argument.
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

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

Project coverage is 72%. Comparing base (5b503b9) to head (27bde52).

Files Patch % Lines
state-chain/custom-rpc/src/lib.rs 0% 22 Missing ⚠️
utilities/src/with_std.rs 0% 10 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #4737    +/-   ##
======================================
- Coverage     72%     72%    -0%     
======================================
  Files        412     412            
  Lines      70705   70580   -125     
  Branches   70705   70580   -125     
======================================
- Hits       51140   50931   -209     
- Misses     17071   17146    +75     
- Partials    2494    2503     +9     

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

@Janislav Janislav changed the base branch from main to epic/lp-api-refactors April 9, 2024 15:16
@Janislav Janislav changed the base branch from epic/lp-api-refactors to main April 11, 2024 12:31
@Janislav Janislav marked this pull request as ready for review April 12, 2024 13:10
@Janislav Janislav requested review from jerryafr, GabrielBuragev and AlastairHolmes and removed request for a team April 12, 2024 13:10
@msgmaxim
Copy link
Contributor

Not sure if this is important to you PR or not, but just FYI, there is a custom implementation for BoostPoolDepth that uses NumberOrHex as a way to serialize the amounts as hex: https://github.com/chainflip-io/chainflip-backend/blob/5b074d547d261097468df780a7e998754d966e7f/state-chain/runtime/src/runtime_apis.rs#L76C78-L77C1. Shouldn't really matter since the result will be the same, but for consistency you might want to change that to use U256 too.

@Janislav Janislav marked this pull request as draft June 18, 2024 10:01
@Janislav
Copy link
Contributor Author

As discussed, putting this here into draft for now @AlastairHolmes.

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