Skip to content

Add alternate transaction object generator#157

Closed
morleyzhi wants to merge 9 commits intostellar:masterfrom
morleyzhi:mz-jsontoxdr
Closed

Add alternate transaction object generator#157
morleyzhi wants to merge 9 commits intostellar:masterfrom
morleyzhi:mz-jsontoxdr

Conversation

@morleyzhi
Copy link
Contributor

@morleyzhi morleyzhi commented Feb 8, 2019

Add StellarBase.makeTransaction, which allows one to generate a transaction from a big JSON blob instead of using the builder pattern. Fixes #158.

Having worked on this, it's possible that this feature request is just a knock against the builder pattern: either we should NOT have stuff like this, since the user can just write the logic themselves, or we should abandon the builder pattern. It doesn't really make sense for every builder in this library to have a non-builder workaround like this. But we shall see.

@tomquisel tomquisel requested a review from bartekn February 8, 2019 22:33
@tomquisel
Copy link
Contributor

@morleyzhi This looks good to me! Just to double-check, though, is this a case that comes up? Do devs ever have a JSON representation of a transaction envelope that they need to convert to XDR? If not, this seems about equivalent to the existing transaction builder, and so maybe not worth it.

@morleyzhi
Copy link
Contributor Author

@bartekn Is this what you were envisioning?

I agree with Tom that this doesn't seem like a really big upgrade, unless there's a standard agreed-upon JSON shape for transactions that people will have. If there's no specific use case, and people just asked for this because they don't like the builder pattern, I think we shouldn't add another API for creating transactions just because people ask.

@tomquisel
Copy link
Contributor

@morleyzhi Thinking about it more, I can see why you'd want a feature like this. It lets you submit transactions from a file or from user input more easily. It gives devs a little domain specific language for making transactions, which is a good thing. I'm down for merging it.

@morleyzhi
Copy link
Contributor Author

@tomquisel The issue I'm struggling with here is that the code is not significantly different:

      // old
      transaction = new StellarBase.TransactionBuilder(source)
        .addOperation(
          StellarBase.Operation.payment({
            destination: destination,
            asset: asset,
            amount: amount
          })
        )
        .addMemo(memo)
        .setTimeout(StellarBase.TimeoutInfinite)
        .build();

      // new
      transaction = StellarBase.makeTransaction({
        sourceAccount: source,
        timeout: StellarBase.TimeoutInfinite,
        memo,
        operations: [
          {
            type: 'payment',
            body: {
              destination,
              asset,
              amount
            }
          }
        ]
      });

They're both domain-specific languages, it's just that the former uses the builder pattern.

I think it'd be a different story if there were a universal JSON standard for transactions, but that doesn't seem to be the case.

As far as the use cases you mentioned: it's also not like someone can just call one function and build-and-submit a transaction, due to the signing workflow. Someone who wants to create transactions from a file is still going to write some imperative scripting code to do that.

The main idea I'm struggling with is having two interfaces that perform the same action, which seems confusing to me. How is someone new to the SDK going to know which interface to use? Are there any hidden trade-offs? Will every class have two interfaces? And if not, why this one?

If it's the builder pattern we have a problem with, we have other options:

  • Change this SDK's API - I strongly disagree with this, since it violates our principle of stability
  • Make some way of all builders automatically having a second, declarative interface - This seems like a lot of work, and could be hard to communicate
  • Create a new, simpler SDK
  • Live with the API we have, warts and all

Even though I've already put the work into this change, I think the last option is the closest to the "stability" value we laid out.

@tomquisel
Copy link
Contributor

@morleyzhi I agree with your points. Having two very similar ways of doing things isn't good. Let's see what @bartekn's thoughts are.

@bartekn
Copy link
Contributor

bartekn commented Feb 12, 2019

@morleyzhi this is good but the community request was something more general. For example, given the following XDR definition of Transaction:

struct Transaction
{
    // account used to run the transaction
    AccountID sourceAccount;

    // the fee the sourceAccount will pay
    uint32 fee;

    // sequence number to consume in the account
    SequenceNumber seqNum;

    // validity range (inclusive) for the last ledger close time
    TimeBounds* timeBounds;

    Memo memo;

    Operation operations<100>;

    // reserved for future use
    union switch (int v)
    {
    case 0:
        void;
    }
    ext;
};

The JSON would look like:

{
    "sourceAccount": "G...",
    "fee": 100,
    "seqNum": 1234,
    "timeBounds": null,
    "memo": {
      "type": MEMO_NONE
    },
    "operations": [
      {
        "sourceAccount": null,
        "type": CREATE_ACCOUNT,
        "destination": "G...",
        "startingBalance": 200000000
      }
    ]
    "ext": 0 // maybe default to zero-value when omitted?
};

This example is for Transaction object but in general you should be able to do the same thing with any XDR object, you check it's definition, write JSON and you can quickly convert it to actual XDR representation. That's why I said the better place would be js-xdr package. There are some things to figure out:

  • How do you represent XDR union, ex:
    union Memo switch (MemoType type)
    {
    case MEMO_NONE:
        void;
    case MEMO_TEXT:
        string text<28>;
    case MEMO_ID:
        uint64 id;
    case MEMO_HASH:
        Hash hash; // the hash of what to pull from the content server
    case MEMO_RETURN:
        Hash retHash; // the hash of the tx you are rejecting
    };
    An obvious way is to use union switch name type. So this would become:
    memo = {
     type: MEMO_TEXT,
     text: "Hello world!"
    }
    But maybe there's a better way? SEP-0011 is responsible for very similar thing but is using a different format than JSON so you can check it out.
  • Some types are just bytes and we need to be able to pass a human-readable representation if there is any. For example, AccountID type (sourceAccount in Transaction) is PublicKey which is really:
    union PublicKey switch (PublicKeyType type)
    {
    case PUBLIC_KEY_TYPE_ED25519:
        uint256 ed25519;
    };
    Would be great to have a helper methods to quickly create byte arrays, ex:
    sourceAccount: AccountId("G..."),
  • Other types require some modifications from human-readable form (ex. all amounts). We should probably have helper functions for this as well.

@morleyzhi
Copy link
Contributor Author

Cool, I didn't think of the XDR angle. I'll think of the best way to do this. (I think among them is to try to change that SEP to choose a JSON representation rather than inventing a new one.)

@bartekn
Copy link
Contributor

bartekn commented Feb 12, 2019

Thanks! This PR is still good if you change JSON structure to actual XDR definition. When js-xdr part is ready we should be able to switch to js-xdr code without introducing any breaking changes.

@tomquisel
Copy link
Contributor

@bartekn can you find the community requests for this feature? I just want us to meet the need correctly.

@morleyzhi:
From my perspective working on StellarX, it's currently very painful in JS/TS to read information out TransactionResult objects. There is fromXDR() that decodes a TransactionResult currently, but it produces deeply nested, confusing objects that contain many _switch, _arm, etc... fields.

I made this (also test cases) hackish XDR parser to make minimal, easy to understand JSON objects from TransactionResult objects. It may be just about right for a general answer, but you may also want to take a different approach. It's just for an example of what makes life easier in a real world situation.

It looks like txrep / SEP-11 does something similar, and could be used to easily generate a JSON object, although it's written in go.

Which brings me to the next point: it may make the most sense to do the XDR -> JSON conversion in the Horizon server when it delivers the TransactionResult and TransactionMeta after transaction submission succeeds. Then we implement the conversion once, and get it for free in all SDKs.

@morleyzhi morleyzhi closed this Feb 20, 2019
@morleyzhi morleyzhi deleted the mz-jsontoxdr branch March 7, 2019 16:42
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.

3 participants