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

add toXDR and fromXDR for multiauth flow #977

Merged
merged 6 commits into from
Jun 11, 2024

Conversation

BlaineHeffron
Copy link
Contributor

Addresses #976

Adds a toXDR and fromXDR for AssembledTransaction class. Also adds a txFromXDR to the Client class. Changes the swap test to use this flow.

Limitations: If you use the XDR multi-auth flow, you must resimulate before the final signAndSend call. This is due to the fact that we can't serialize the XDR of the transaction envelope with the results of the simulation. If we want to do this, we would need to add an AssembledTransaction type to XDR in Stellar Base.

Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

In the CHANGELOG entry that you add for this, let's be sure to mention that toJSON and fromJSON and txFromJson are now all deprecated, and to prefer the XDR variants.

If it were up to me, given the low-usage of this so far (literally probably only the CLI using this toJSON and fromJSON logic right now), I would just ship a breaking change. But we should probably wait until the next major version to do that. Maybe we can also add @deprecated markers to the JSDoc for these json-related methods.

@BlaineHeffron BlaineHeffron marked this pull request as ready for review June 5, 2024 23:04
@BlaineHeffron BlaineHeffron requested a review from chadoh June 5, 2024 23:04
@stellar stellar deleted a comment Jun 6, 2024
@Shaptic
Copy link
Contributor

Shaptic commented Jun 6, 2024

If it were up to me, [...] I would just ship a breaking change. But we should probably wait until the next major version to do that.

I completely agree, but it's not the way we want to do things anymore (i.e. we want a single major version across the stack [up to the platform] to define protocol compatibility). Yet another reason why the SDK and the binding code should be able to evolve independently 😉

src/contract/assembled_transaction.ts Outdated Show resolved Hide resolved
src/contract/assembled_transaction.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

Glad to have this, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants