-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: main
Are you sure you want to change the base?
Conversation
fcf2e4f
to
7721f3c
Compare
There was a problem hiding this 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:
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 ) |
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
|
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. |
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? |
@leighmcculloch Looking into this, we can use 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,
},
}
|
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. |
@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")] |
There was a problem hiding this 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 🚗
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. |
@willemneal Is this needed still? |
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:
Close #16