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

txnbuild: build transactions using xdr string #1329

Closed
poliha opened this issue May 24, 2019 · 5 comments
Closed

txnbuild: build transactions using xdr string #1329

poliha opened this issue May 24, 2019 · 5 comments
Assignees
Labels
txnbuild 2nd-generation transaction build library for Go SDK

Comments

@poliha
Copy link
Contributor

poliha commented May 24, 2019

The JS SDK enables users to intialise a transaction object using the base64 transaction envelope.
It will be good to have something similar for the Go sdk.

@poliha poliha added the txnbuild 2nd-generation transaction build library for Go SDK label May 24, 2019
@ire-and-curses
Copy link
Contributor

I looked into exactly what's involved in this. TLDR: we should just do it now.

The old Go build library takes an encoded XDR transaction string as an optional argument, and then uses the xdr.SafeUnmarshalBase64() method to deserialise to XDR-lib structs. Because all methods of that library work directly on these XDR struct representations, you get this capability to initialise from XDR for free (there's no higher level object model to map to).

The new txnbuild library uses a higher level abstraction for Transaction, Operation, Memo etc so that the user can initialise and modify simple Go structs in a way that is more intuitive for network use. This means that to initialise from an encoded XDR transaction requires a mapping from XDR structs to the corresponding higher-level objects, else the library's assumptions about state are violated.

I originally thought we should defer this until we start using go-xdr, because that code supports some of this already, and automatically. However we still have to handle mapping from that library to txnbuild, or deprecate certain txnbuild structs, which is a separate, bigger issue.

This mapping is handled in the JS SDK for Transactions here. It delegates to the appropriate mappings for each XDR-sub-object. The majority of the work is to handle each kind of Operation (see here).

The code is a little tedious, but straightforward. We should just implement the equivalent thing for txnbuild. To do this,

  • We add a FromXDR() method for each high level object (i.e. each Operation, Memo, Timeboundsand Account).
  • Transaction.FromXDR() orchestrates the sub-XDR objects in a similar way to the JS implementation. Essentially this acts like an alternative constructor.

The goal is that after calling one of these methods the object should be able to be used normally, e.g. to have signers added, timebounds tweaked etc, and then built when ready.

This work adds full support for building from XDR strings, which is a significant feature improvement. It will make implementation of #1530 much simpler, and makes #1540 unnecessary.

@OwenJacob
Copy link
Contributor

A slightly related question: The txnbuild.Transaction struct holds the network string. When this get's built into the xdr.TransactionEnvelope this seems to be dropped so that when we decode the envelope we have to re-add the network string before signing. What's the rationale behind this?

As it stands this means a signing service needs to know which network the transaction is destined for to add a signature. Should adding a signature require knowledge of the network passphrase?

@poliha
Copy link
Contributor Author

poliha commented Jul 26, 2019

@ire-and-curses thanks for looking into this. I think we should be able to include this in the next release. I agree with most of what you have outlined, my only concern is the following:

The goal is that after calling one of these methods the object should be able to be used normally, e.g. to have signers added, timebounds tweaked etc, and then built when ready.

I don't think we should be encouraging users to modify transactions(except adding signers) that has already been built. The output of FromXDR should only be inspected for correctness before adding a signature.
UPDATE: Fixing #1448 would probably help with this.

In any case, a transaction that is modified after it has been signed, will fail when submitted to the network. I guess modification of an XDR string is only useful if the transaction has not been signed.

@OwenJacob the network passphrase is used to generate the hash of the transaction. The hash is what is signed. This is why it is required for the signing process.

@ire-and-curses
Copy link
Contributor

Agree that if the incoming serialised TX is signed we should prevent Build() from overwriting the XDR, since that is not what the originator would have intended.

For the case of an unsigned incoming serialised TX, we should allow tweaking and building as usual. This handles the use cases of third party signing, and also users storing a TX in serialised form for reference or transport.

@poliha
Copy link
Contributor Author

poliha commented Aug 6, 2019

implemented in #1545
tests in #1555 and #1558

@poliha poliha closed this as completed Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
txnbuild 2nd-generation transaction build library for Go SDK
Projects
None yet
Development

No branches or pull requests

3 participants