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

fix(near-primitives): EpochReference enum can be flattened into a struct #4848

Merged
merged 3 commits into from
Sep 21, 2021

Conversation

miraclx
Copy link
Contributor

@miraclx miraclx commented Sep 20, 2021

EpochReference is defined as such:

#[derive(Serialize, Deserialize, Debug)]
#[serde(rename_all = "snake_case")]
pub enum EpochReference {
EpochId(EpochId),
BlockId(BlockId),
Latest,
}

And it's serialization, as such:

% EpochReference::BlockId(...) EpochReference::Latest
expectation {"block_id": ...} {"latest": null}
reality {"block_id": ...} "latest"

As you can already see, serde's default method for serializing unit variants is to use a string
unlike how it uses an object for the rest of the variants.

This PR implements custom serialization for EpochReference, specifically for the Latest variant. So it serializes to {"latest": null}.

Note: this retains the default deserialization so we have no breaking changes on the deserialization side.

So, the following values properly deserialize into that variant:

  • "latest"
  • {"latest": null}
  • {"latest": []}

props to @volovyk-s 🎉 for discovering this while using the new near-jsonrpc-client

@miraclx
Copy link
Contributor Author

miraclx commented Sep 20, 2021

More context:

In the case where it's embedded in a struct whose serialization requires flattening EpochReference, for example:

#[derive(Serialize, Deserialize, Debug)]
pub struct RpcValidatorRequest {
#[serde(flatten)]
pub epoch_reference: near_primitives::types::EpochReference,
}

All is well until you use the Latest variant of EpochReference.

code expectation reality
RpcValidatorRequest {
    epoch_reference: EpochReference::BlockId(...)
};
{
  "block_id": ...
}
RpcValidatorRequest {
    epoch_reference: EpochReference::Latest
};
{
  "latest": null
}
Error: can only flatten structs and maps (got an enum)

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

Meh, this somehow slipped through my review, but the API is quite odd. I am fine with this hotfix, though I want to share more context here.

validators({block_id: 123})
validators({block_id: "deadbeef"})
validators({latest: null})

We have not even updated the documentation: https://docs.near.org/docs/api/rpc/network#validation-status, so we may as well refactor the API altogether.

validators({epoch_reference: {block_id: 123}})
validators({epoch_reference: {block_id: "deadbeef"}})
validators({epoch_reference: "latest"})

Maybe you have a better idea, I am currently not at my highest with naming 😞

@near-bulldozer near-bulldozer bot merged commit 4306275 into master Sep 21, 2021
@near-bulldozer near-bulldozer bot deleted the fix-serialize-epoch-reference branch September 21, 2021 10:33
@matklad
Copy link
Contributor

matklad commented Sep 21, 2021

It's not immediately obvious that derive serialization can deserialize our custom one, so it might make sense to add a unit-test for this.

nikurt pushed a commit that referenced this pull request Sep 30, 2021
…truct (#4848)

`EpochReference` is defined as such:

https://github.com/near/nearcore/blob/06d05bce3fab578a91b210d7354fb17427fcc9d6/core/primitives/src/types.rs#L1001-L1007

And it's serialization, as such:

| % | `EpochReference::BlockId(...)` | `EpochReference::Latest` |
| - | ------------------------------ | ------------------------ |
| expectation | `{"block_id": ...}` | `{"latest": null}`
| reality | `{"block_id": ...}` | `"latest"` |

As you can already see, `serde`'s default method for serializing unit variants is to use a string 
unlike how it uses an object for the rest of the variants.

This PR implements custom serialization for `EpochReference`, specifically for the `Latest` variant. So it serializes to `{"latest": null}`.

> Note: this retains the default deserialization so we have no breaking changes on the deserialization side.
>
> So, the following values properly deserialize into that variant:
> - `"latest"`
> - `{"latest": null}`
> - `{"latest": []}`

_props to @volovyk-s 🎉  for discovering this while using the new [`near-jsonrpc-client`](https://github.com/near/near-jsonrpc-client-rs)_
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.

5 participants