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

Rearchitect or improve build package #845

Closed
bartekn opened this issue Jan 30, 2019 · 1 comment
Closed

Rearchitect or improve build package #845

bartekn opened this issue Jan 30, 2019 · 1 comment

Comments

@bartekn
Copy link
Contributor

bartekn commented Jan 30, 2019

This issue describes the limitations and drawbacks of the current design of build package. I'm not sure if we want to rearchitect it or just improve the current state - we should discuss it here.

  • Around 50% of methods accept interface{} type argument. It's not clear what to use, it's super easy to pass something that doesn't match (and it won't be detected at compilation time).
  • Operation builders are really hard to use as it's not clear which parameters are required and which optional. Example: Payment(muts ...interface{}) (result PaymentBuilder). It should be clear how to use by looking at the method definition - without checking the docs. So probably something like: Payment(destination string, amount string, asset Asset, opts ...PaymentMutator) or:
    AddOperation(Payment{
        Destination: "G...",
        Amount: "1",
        Asset: Asset.native(),
        Opts: []PaymentMutator{OperationSource("G...")},
    })
    could be simpler.
  • Exactly the same thing as above but when creating transaction.
  • It's not possible to build a transaction builder from a serialized transaction. Ex. I want to add a new/modify/remove operation or add a second signature.
  • Some important features (like setting timeout - Transaction included in the ledger after a couple minutes since submission stellar-core#1811) are missing.
  • Maybe we should add some nice wrapper functions for xdr package structs and use them instead of mirroring transaction related structs here?
  • New signature types are not supported at all.
  • Saving errors on struct instead of returning them. This results in a code that makes it super easy to forget to handle errors (and you can't catch it using static analysis):
    someObject.SomeFunction() // Error stored in `someObject.Err`
    // You need to remember to check someObject.Err here
    // as SomeFunction() definition says nothing about errors.
    It was partially fixed in Return errors in builder functions #250 but there are still some instances of this in public API and internal calls.
@ire-and-curses
Copy link
Member

After a lot of discussion, we decided to produce a new library to address these concerns. txnbuild is now released. It is the replacement for build, and resolves the problems described in this issue.

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

No branches or pull requests

2 participants