Transaction builder class rebased

[esaminu]

Motivation

Updated and rebased #891

Description

Adds builder pattern transaction creation:

bob.createTransaction("alice.near").transfer("1000000000000000000").signAndSend();

Checklist

• Read the contributing guidelines
• Commit messages follow the conventional commits spec
• Performed a self-review of the PR
• Added automated tests
• Manually tested the change

[changeset-bot]

🦋 Changeset detected

Latest commit: e58229d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
near-api-js	Minor
@near-js/cookbook	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[morgsmccauley]

Looks good, just a few comments :)

[andy-haynes]

The refactor here seems a little heavy for what it accomplishes, I'd prefer if we explored implementing a "dumb" TransactionBuilder instead of one that overlaps functionality in the Account class.

[andy-haynes]

Rather than refactor the Account class to share some functionality, why not pass the signTransaction and signAndSendTransaction methods to the new class? TransactionBuilder doesn't need to know how to send transactions, as evidenced by the name 🙂

Passing the methods would eliminate the need to break up the Account class, which seems to have been done solely for the purpose of factoring out two methods (and their dependencies). What would you think about something like this instead?

createTransaction(receiverId: string): TransactionBuilder {
  return new TransactionBuilder({
    signTransaction: (actions: Action[]) => this.signTransaction(this.accountId, actions),
    signAndSendTransaction: (actions: Action[]) => this.signAndSendTransactions({ receiverId: this.accountId, actions }),
  });
}
...
class TransactionBuilder {
  signTransaction: (actions: Action[]) => Promise<[Uint8Array, SignedTransaction]>;
  signAndSendTransaction: (actions: Action[]) => Promise<FinalExecutionOutcome>;

  constructor({ signTransaction, signAndSendTransaction }) {
    this.signTransaction = signTransaction;
    this.signAndSendTransaction = signAndSendTransaction;
  }
  ...
}

Now TransactionBuilder doesn't need a connection or accountId - it just builds transactions and exposes sign and signAndSend methods that look like

sign(): Promise<[Uint8Array, SignedTransaction]> {
  return this.signTransaction(this.actions);
}

With this the refactor is not required and we can keep Account logic consolidated.

[esaminu]

This is an interesting idea but it makes Account the only possible consumer of TransactionBuilder since there is only one way we'd like to send transactions (by caching accessKeys and hitting rpc with exponential backoff) and that way would reside solely in Account unless the user decided to reimplement it. Users and other classes are more likely to have a Connection instance, senderId and receiverId than an implementation for signing transactions and sending them.

Since we'd like to hold this implementation of sending transactions constant it is not specific to Account instances and should either reside elsewhere or at least as a static method. That way it can be reused independently of Account in future by other implementations attempting to sign and send transactions. Separating out how Account sends transactions from Account specific functionality has been overdue imo.

[andy-haynes]

I see what you mean about its use outside of Account, thanks for the clarification.

Separating out how Account sends transactions from Account specific functionality has been overdue imo.

I think you're 100% right on this, however I would argue that factoring this behavior out into a parent class doesn't go far enough in separating these concerns. Inheritance may be more convenient in the short term for this but we have an opportunity to use composition to accomplish the same thing in a more robust way. By passing instances of TransactionSender to Account and TransactionBuilder rather than putting this in a parent class, we can avoid the coupling that inheritance guarantees.

Another benefit is that mocking becomes trivial - different TransactionSender mocks (e.g. one for throwing an error, another returning unexpected data, etc.) could be shared across unit tests, eliminating the need for transaction signing integration tests on consumers of TransactionBuilder.

[esaminu]

Great points. I rebased this PR on #1018 and #1019 👍

Not needed