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!: Incorporate upstream breaking changes and update stellar deps #12

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

Conversation

willemneal
Copy link
Member

@willemneal willemneal commented Sep 15, 2024

See breaking changes here:
stellar/stellar-rpc#280

As per @leighmcculloch's point in #16 , the types were updated to be backwards compatible. Any removed fields are ignored and new fields are Optional.

Still needed:

  • sample responses from upstream

Close #16

Cargo.lock Outdated Show resolved Hide resolved
@willemneal willemneal changed the title feat: update xdr and stellar-strkey feat!: Incorporate upstream breaking changes and update stellar deps Oct 2, 2024
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up early a few weeks ago 👏🏻 ❤️.

I started reviewing, but stopped, because it seems like there's more changes in this PR than are required, probably from a misunderstanding of a peculiarity in the RPC Go code it's attempting to mirror. It looks like we're trying to guess what the new API is by looking at the RPC changes, rather than a document that describes what the diff between the old and new API.

I suggest we code to the document that the Platform team wrote about what the API changes are, that's copied into this issue:

Cargo.lock Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@2opremio
Copy link
Contributor

2opremio commented Oct 4, 2024

How is this coming along? We are about to release RPC rc1 and end-to-end tests are failing because of this (see https://github.com/stellar/soroban-rpc/actions/runs/11171362010/job/31055922343?pr=308 )

@2opremio 2opremio closed this Oct 4, 2024
@2opremio 2opremio reopened this Oct 4, 2024
@leighmcculloch
Copy link
Member

We are about to release RPC rc1 and end-to-end tests are failing because of this (see stellar/soroban-rpc/actions/runs/11171362010/job/31055922343?pr=308 )

The system tests won't pass until the CLI is updated I think? The CLI also runs system tests, so each time we break them it's like this chicken and egg problem.

classDiagram
    direction RL
    RPC <-- RPCClient
    RPCClient <-- CLI
    CLI <-- SystemTest
    SystemTest <-- RPC
    RPC <-- QuickStart
    QuickStart <-- SystemTest
    SystemTest <-- CLI
Loading

@willemneal
Copy link
Member Author

@2opremio @leighmcculloch

Can we use the getNetwork endpoint to know the API? Then we could have and From trait to convert the responses.

And @2opremio besides the TransactionInfo flattening do the new types reflect the API. It seemed to me that there were more changes than the document @leighmcculloch is referencing in his issue.

@leighmcculloch
Copy link
Member

leighmcculloch commented Oct 4, 2024

Can we use the getNetwork endpoint to know the API?

That would mean calling getNetwork with every other call in a batch request... It seems like an uncommon pattern, but also I guess that would work.

Is there a reason we can't just make the responses decode both forms/structures at once?

@willemneal
Copy link
Member Author

@leighmcculloch Looking into this, we can use #[serde(untaged)] with an enum that contains the differing fields. Then use #[serde(flatten)] in the top level struct. I will give this a try.

E.g.

use serde::{Deserialize, Serialize};

#[derive(Deserialize, Serialize, Debug)]
struct ResponseType {
    common_field: String,
    #[serde(flatten)]
    variable_field: DifferentResponses,
}

#[derive(Deserialize, Serialize, Debug)]
#[serde(untagged)]
enum DifferentResponse {
    OldResponse {
        old_field: String,
    },
    NewResponse {
        new_field: bool,
    },
}

#[serde(flatten)] will also fix the TransactionInfo issue.

@leighmcculloch
Copy link
Member

with an enum that contains the differing fields

Nice. Can we do one enum for each field? I think that's more true to how we want serde to fill and select fields.

@leighmcculloch
Copy link
Member

leighmcculloch commented Oct 4, 2024

@willemneal For fields which are just a rename, we can use aliasing in serde, see: https://serde.rs/field-attrs.html

#[serde(alias = "old", alias = "new")]

@willemneal willemneal marked this pull request as ready for review October 7, 2024 19:36
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

quick drive-by review 🚗

Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@leighmcculloch
Copy link
Member

Because the RPC wound back it's changes, and are first deprecating old fields then adding new fields, we can reduce this PR to adding the new fields.

@leighmcculloch
Copy link
Member

@willemneal Is this needed still?

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.

Changes to support the API changes upcoming in rpc v22
5 participants