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: Wrap eth::U256 into a type #2086

Open
sunce86 opened this issue Nov 27, 2023 · 3 comments
Open

feat: Wrap eth::U256 into a type #2086

sunce86 opened this issue Nov 27, 2023 · 3 comments
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@sunce86
Copy link
Contributor

sunce86 commented Nov 27, 2023

Problem

Default serde implementation for U256 type encodes/decodes in hexadecimal format, while we always want to use decimal format. We need to make sure that every time we use U256, we add an annotation #[serde_as(as = "serialize::U256")] to the field.

This has been forgotten numerous times, cf #2075, #1958, etc...

Suggested solution

Wrap U256 in its own type that implements default serialization using HexOrDecimalU256.

Additionally, investigate if there is a way to forbid using raw U256 in our system (with clippy or something similar) so we don't accidentally continue to U256 instead of our own type.

@sunce86 sunce86 added the good first issue Good for newcomers label Nov 27, 2023
@fleupold fleupold added the help wanted Extra attention is needed label Jan 16, 2024
@duguorong009
Copy link

@sunce86
Can I pick the issue?

@duguorong009
Copy link

@sunce86 @fleupold
Sorry, but I think I can't resolve the issue.

I've tried several times, in order to implement the suggested solution.
But, unfortunately, I didn't manage to finish it.
The reason is that the codebase is HUGE(at least for myself), and
the introduction of new wrapper type brings too many updates which I can't handle.

From my side, I think it is better way that the team should be more diligent when reviewing the PR which includes U256 type.
Or, hopefully, another guy can resolve this issue as you expect.

@MartinquaXD
Copy link
Contributor

Thanks for trying, though.
I think the biggest problem is probably that in the orderbook and autopilot we use the same types that define the API and the deserialization behavior in the business logic. This makes any change to the API type also a change to the business logic which is as you pointed out quite a lot.
We didn't make this mistake in the driver and solvers crates so there it should be a reasonably small change.
In the medium term future we should probably apply the same pattern to the orderbook and autopilot so we can easily change the deserialization without affecting the whole code base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants