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

Improve DesmosQuery and DesmosMsg with serde tag #21

Closed
dadamu opened this issue Mar 11, 2022 · 3 comments · Fixed by #23
Closed

Improve DesmosQuery and DesmosMsg with serde tag #21

dadamu opened this issue Mar 11, 2022 · 3 comments · Fixed by #23
Assignees

Comments

@dadamu
Copy link
Contributor

dadamu commented Mar 11, 2022

Currently the structure of DesmosMsg is like:

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
#[serde(rename_all = "snake_case")]
pub struct DesmosMsg {
    pub route: DesmosRoute,
    pub msg_data: DesmosMsgRoute,
}

It could be simplified with serde tag attribute, say:

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
#[serde(rename_all = "snake_case", tag = "route", content = "msg_data")]
pub enum DesmosMsg {
    Profiles(ProfilesMsg),
    Subspaces(SubspacesMsg),
}

To produce the json like:

{
    "route": "subspace",
    "msg_data": {
        "create_subspace": {
            "name": "test",
            "description": "test",
            "......"
        }
    }
}

We can do the same way to DesmosQuery as well.
It will reduce some code related to DesmosRoute. What do you think? @bragaz

Note:

it will break the current wasm part of modules in the desmos chain since the json format is different from the current one.

Reference: https://serde.rs/enum-representations.html#adjacently-tagged

@dadamu
Copy link
Contributor Author

dadamu commented Mar 14, 2022

BTW, I have created a branch for testing it.
https://github.com/desmos-labs/desmos-contracts/tree/paul/refactor-route

@leobragaz
Copy link
Collaborator

Nice one! This rename feature from serde is very powerful! I guess we can make this change, but we need to add also some docs lines in the file to let others that are not familiar with serde to properly understand the signature (maybe you can put the reference you put in the note directly?).

@dadamu
Copy link
Contributor Author

dadamu commented Mar 14, 2022

@bragaz Sure, will do it with a PR.

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 a pull request may close this issue.

2 participants