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

Support two sorobans for preflight #264

Open
wants to merge 7 commits into
base: v22-breaking-changes
Choose a base branch
from

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Aug 10, 2024

This is a companion patch to stellar/rs-soroban-env#1442 to support two sorobans in the RPC the same way that PR requires us to support two in stellar-core.

Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@dmkozh dmkozh left a comment

Choose a reason for hiding this comment

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

Thanks, this approach looks much better to me much better maintenance- and accuracy-wise (even if a bit hacky) - looking forward to rebase (and adding the missing file?).

cmd/soroban-rpc/lib/preflight/src/lib.rs Show resolved Hide resolved
@Shaptic Shaptic mentioned this pull request Aug 13, 2024
19 tasks
@graydon graydon marked this pull request as ready for review August 13, 2024 23:01
// This is the same limit as the soroban serialization limit
// but we redefine it here for two reasons:
//
// 1. To depend only on the XDR crate, not the soroban host.
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a brief unresolved discussion with @leighmcculloch when implementing this feature (#249 (review)) that I think is worth resurfacing here:

When RPC has both the P22 and P23 host loaded for preflight (if I understand correctly, this means P23 is "compiled in" but is not being used until the upgrade kicks in), we want to make sure that we continue to return the JSON schema for P22 definitions. When the upgrade kicks in, we want to switch to P23 definitions (as well as P23 preflight itself). Does that mean we need the same adapter-esque code here to not always rely on curr when doing the XDR -> JSON translation?

Also, just to sanity check, we don't need to add support to the RPC (Go) side of preflight to perform this selection once its ingested the upgrade ledger, right? Since this is implied by the various let proto = ledger_info.protocol_version; lines in preflight.

But that does mean we need to perform the module swap on the RPC side for translation, since there's no such version check?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separately, I think this should target our P22 branch v22-breaking-changes.

Copy link
Member

@leighmcculloch leighmcculloch Aug 20, 2024

Choose a reason for hiding this comment

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

It's not obvious to me that the version of the XDR JSON schema should be tied to the protocol upgrade. At least we should consider the options.

When an upgrade occurs from v22 to v23, the change is to what features we will see show up in the XDR, such as a new enum variant becomes available for use.

What happens today (@Shaptic I realise you already know this, I'm just getting my thoughts out) is that the structure of the XDR / JSON changes out-of-band of protocol upgrades.

For example, when new XDR is released for an upcoming protocol, Stellar SDKs start adopting the new XDR structure before the protocol upgrade occurs.

Software first adopts the new XDR structure, and then new elements of that structure become in use after the protocol upgrade.

Continuing that existing pattern seems worth considering. i.e. To say that v23 of stellar-rpc uses the v23 XDR structure and therefore the structure of the output of v23 stellar-rpc is the same as v23 *-stellar-sdk.

I recognise that the JSON is the first time the structure, and not just the binary format, has to be coordinated, so what we are doing today with the structure really doesn't have to have any bearing on where we go from here.

But I think applications that need to update to the new XDR structure will need to do so before a protocol upgrade so there's one class of participants who will still want to have today's workflow.

If we were to do this, I think it would mean encoding the version of the stellar-rpc into a field of all responses, so that anyone parsing the responses could first check that metadata to confirm what version of rpc the response has been generated from. With any HTTP API it'd be reasonable, and preferred, to communicate the version separately in a header rather than in the response body, but I guess with JSON-RPC it will probably be in the body.

--

At some point @accordeiro suggested, could we make the JSON parsing backwards compatible, so that the Rust XDR Lib when the structure is updated we add in custom deserialisation logic that captures if the old format was in use. In theory this should be possible because the structural changes that typically take place are rather simple. Such as adding a new enum, or new union arm, or converting an optional into an enum or union. We'd need to allocate a small effort to spike out what's possible with the serde crate we use for JSON serialization as we have no experience with custom deserialization of this kind. (cc @janewang)

Copy link
Contributor

Choose a reason for hiding this comment

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

But I think applications that need to update to the new XDR structure will need to do so before a protocol upgrade so there's one class of participants who will still want to have today's workflow.

Is there a real expectation to support the schema at all from a programmatic perspective? There's no way today to turn that into a usable XDR structure, anyway. It's basically a visual convenience mechanism that devs shouldn't really rely on at all. I think we should have this discussion in parallel as we decide how this feature should be used and not let it block P22 work, so I'll be merging this 🫡

@graydon graydon changed the base branch from main to v22-breaking-changes August 30, 2024 05:06
@graydon
Copy link
Contributor Author

graydon commented Aug 30, 2024

I've rebased this on the v22-breaking-changes branch and fixed as much of the CI issues as I can; it's currently failing on a mismatch with the stellar/go monorepo's XDR version, which I do not feel qualified to be adjusting (I don't even know what the policy is for doing that, much less how this relates to the v22-breaking-changes branch).

@Shaptic is this close enough that you can take it over the finish line now? I don't really understand what the conclusion of your conversation above is with @leighmcculloch -- if it's not quite right yet, I'm happy to make any further changes if you can be specific!

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