Enums camel case serialization #2056

squadgazzz · 2023-11-14T13:46:44Z

Description

Copied from the original PR:

Replaced all occurrences of #[serde(rename_all = "lowercase")] with `#[serde(rename_all = "camelCase")].
Ensured that the changes maintain the readability and functionality of the code.
This pull request addresses the concern raised about enum variant readability and serialisation. The changes improve the codebase's consistency and enhance its clarity.
Please review the changes, and I'm open to any feedback or adjustments to ensure the code's quality and maintainability. Thank you for your time and consideration.

Related PRs

#2020

fixes: #2016

# Conflicts: # crates/driver/src/infra/solver/dto/notification.rs # crates/solvers/src/api/routes/notify/dto/notification.rs

github-actions · 2023-11-14T13:46:59Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

squadgazzz · 2023-11-14T13:51:34Z

I have read the CLA Document and I hereby sign the CLA

MartinquaXD

Nice, thank you!

BTW if you write fixes: #SOME_ISSUE_NUMBER in the PR description it will auto-close that issue when the PR gets merged. I believe this will not happen if you only list it in the related issues section.

sunce86

Nice!
I see a few more occurrences of lowercase in our codebase. Can we substitute those also (with the exception of the ones related to database)?

sunce86 · 2023-11-15T10:14:29Z

@anxolin can someone from the frontend team check if this PR breaks the API? namely changes around signingScheme

fleupold

I think we should only rename the "new" APIs where we currently still control both sides of the communication. We shouldn't make any breaking changes to the public facing orderbook API (it's not worth the effort of rolling this out) and also not to the APIs we are consuming.

Let's not change the crates/model, crates/orderbook and any infra where the upstream is not one of our services.

fleupold · 2023-11-18T10:00:21Z

crates/shared/src/tenderly_api.rs

@@ -169,7 +169,7 @@ pub struct SimulationRequest {
 }

 #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
-#[serde(rename_all = "lowercase")]


We don't control the API, so this may break.

fleupold · 2023-11-18T10:02:15Z

crates/solvers/src/infra/dex/balancer/dto.rs

@@ -9,7 +9,7 @@ use {
 };

 #[derive(Debug, Serialize)]
-#[serde(rename_all = "lowercase")]


I believe this API is also maintained by balancer and thus may break (but given the non compound words, it may not be an issue here)

fleupold · 2023-11-18T10:03:34Z

crates/solvers/src/tests/zeroex/market_order.rs

@@ -88,7 +88,7 @@ async fn sell() {
    }])
    .await;

-    let engine = tests::SolverEngine::new("zeroex", zeroex::config(&api.address)).await;
+    let engine = tests::SolverEngine::new("zeroEx", zeroex::config(&api.address)).await;


nit: we don't really need to change this, do we?

fleupold · 2023-11-18T10:04:11Z

crates/solvers/src/tests/paraswap/market_order.rs

@@ -148,7 +148,7 @@ async fn sell() {
    ])
    .await;

-    let engine = tests::SolverEngine::new("paraswap", paraswap::config(&api.address)).await;
+    let engine = tests::SolverEngine::new("paraSwap", paraswap::config(&api.address)).await;


also no need to change this (I think we at some point decided to have all solver names lower case).

This reverts commit 2a6a9b6.

anxolin

@anxolin can someone from the frontend team check if this PR breaks the API? namely changes around signingScheme

@sunce86 We would need a bit more of context to test this. Is this really a breaking change? (or it doesn't affect to the requests we are doing and is just affecting the swagger types?

Also, i don't see any change in the swagger for the normal API, but i see changes in the one for solvers. Is it possible you forgot to generate the swagger? @mfw78 do we generate it dynamically or is still manual?

fleupold · 2023-11-22T14:26:53Z

It shouldn't be a breaking change (this is what we need to test).
I think the right way to test this is to run cowswap and the orderbook locally and make sure we can still get quotes and place orders. @squadgazzz do you think you can do this? We will then still have time to see issues in barn before this goes into prod.

anxolin · 2023-11-22T16:00:28Z

Here are some instructions on how to point the web to your local API https://github.com/cowprotocol/cowswap#orderbook-api-endpoints

mfw78 · 2023-11-22T16:32:01Z

Also, i don't see any change in the swagger for the normal API, but i see changes in the one for solvers. Is it possible you forgot to generate the swagger? @mfw78 do we generate it dynamically or is still manual?

Still manual as far as I'm aware.

squadgazzz · 2023-12-13T15:33:40Z

I think the right way to test this is to run cowswap and the orderbook locally and make sure we can still get quotes and place orders. @squadgazzz do you think you can do this? We will then still have time to see issues in barn before this goes into prod.

Tested locally with cowswap and explorer, didn't encounter an issue.

devansh1401 and others added 11 commits October 26, 2023 15:12

Update serde serialization to use camelCase

90716f5

Merge branch 'cowprotocol:main' into main

2b612bb

Retrigger CI checks

e604c49

Retrigger CI checks

149e162

Merge branch 'main' of https://github.com/devansh1401/services

8f979ca

retrigger

7caca35

Merge branch 'main' into main

dbfb915

retrigger

5b2c844

Merge branch 'main' of https://github.com/devansh1401/services

1ba1a8f

Merge remote-tracking branch 'origin/main' into devansh1401/main

28e0a6a

# Conflicts: # crates/driver/src/infra/solver/dto/notification.rs # crates/solvers/src/api/routes/notify/dto/notification.rs

Fix unit tests and fmt

df7e767

github-actions bot added a commit that referenced this pull request Nov 14, 2023

@squadgazzz has signed the CLA from Pull Request #2056

861e8f0

Align with solve crate case style

e01e0dd

squadgazzz marked this pull request as ready for review November 14, 2023 17:53

squadgazzz requested a review from a team as a code owner November 14, 2023 17:53

MartinquaXD approved these changes Nov 14, 2023

View reviewed changes

sunce86 reviewed Nov 15, 2023

View reviewed changes

Migrated remaining enums

2a6a9b6

fleupold reviewed Nov 18, 2023

View reviewed changes

squadgazzz added 3 commits November 20, 2023 09:57

Revert "Migrated remaining enums"

13ff721

This reverts commit 2a6a9b6.

Merge branch 'main' into enums-camel-case-serialization

3441400

Revert breaking changes

049a592

sunce86 mentioned this pull request Nov 20, 2023

Replace #[serde(rename_all = "lowercase")] with #[serde(rename_all = "camelCase")] #2020

Closed

Migrate remaining enums

1a33c98

squadgazzz force-pushed the enums-camel-case-serialization branch from 569b619 to 1a33c98 Compare November 20, 2023 11:25

fleupold approved these changes Nov 20, 2023

View reviewed changes

sunce86 approved these changes Nov 22, 2023

View reviewed changes

anxolin reviewed Nov 22, 2023

View reviewed changes

Merge branch 'main' into enums-camel-case-serialization

cce3f07

squadgazzz enabled auto-merge (squash) December 13, 2023 15:34

squadgazzz merged commit a843533 into main Dec 13, 2023
8 checks passed

squadgazzz deleted the enums-camel-case-serialization branch December 13, 2023 15:39

github-actions bot locked and limited conversation to collaborators Dec 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enums camel case serialization #2056

Enums camel case serialization #2056

squadgazzz commented Nov 14, 2023 •

edited

Loading

github-actions bot commented Nov 14, 2023 •

edited

Loading

squadgazzz commented Nov 14, 2023

MartinquaXD left a comment

sunce86 left a comment •

edited

Loading

sunce86 commented Nov 15, 2023

fleupold left a comment

fleupold Nov 18, 2023

squadgazzz Nov 20, 2023

fleupold Nov 18, 2023

fleupold Nov 18, 2023

squadgazzz Nov 20, 2023

fleupold Nov 18, 2023

squadgazzz Nov 20, 2023

anxolin left a comment •

edited

Loading

fleupold commented Nov 22, 2023

anxolin commented Nov 22, 2023

mfw78 commented Nov 22, 2023

squadgazzz commented Dec 13, 2023

Enums camel case serialization #2056

Enums camel case serialization #2056

Conversation

squadgazzz commented Nov 14, 2023 • edited Loading

Description

Related PRs

github-actions bot commented Nov 14, 2023 • edited Loading

squadgazzz commented Nov 14, 2023

MartinquaXD left a comment

Choose a reason for hiding this comment

sunce86 left a comment • edited Loading

Choose a reason for hiding this comment

sunce86 commented Nov 15, 2023

fleupold left a comment

Choose a reason for hiding this comment

fleupold Nov 18, 2023

Choose a reason for hiding this comment

squadgazzz Nov 20, 2023

Choose a reason for hiding this comment

fleupold Nov 18, 2023

Choose a reason for hiding this comment

fleupold Nov 18, 2023

Choose a reason for hiding this comment

squadgazzz Nov 20, 2023

Choose a reason for hiding this comment

fleupold Nov 18, 2023

Choose a reason for hiding this comment

squadgazzz Nov 20, 2023

Choose a reason for hiding this comment

anxolin left a comment • edited Loading

Choose a reason for hiding this comment

fleupold commented Nov 22, 2023

anxolin commented Nov 22, 2023

mfw78 commented Nov 22, 2023

squadgazzz commented Dec 13, 2023

squadgazzz commented Nov 14, 2023 •

edited

Loading

github-actions bot commented Nov 14, 2023 •

edited

Loading

sunce86 left a comment •

edited

Loading

anxolin left a comment •

edited

Loading