Proposal: uniformize transaction factories

[andreibancioiu]

Summary

Following the design of TokenOperationsFactory, re-structure the other transaction factories and builders.

Current state

There are a number of design inconsistencies among the components responsible with constructing ready-to-sign transactions.

Creating a TokenTransfersFactory, then a number of transfer transactions:

const factory = new TransferTransactionsFactory(new GasEstimator());
const txA = factory.createEGLDTransfer({ ... });
const txB = factory.createESDTTransfer({ ... });

Creating a TokenOperationsFactory, then a number of transactions:

const factory = new TokenOperationsFactory(new TokenOperationsFactoryConfig("D"));
const txA = factory.issueFungible({ ... });
const txB = factory.issueNonFungible({ ... });

Creating a SmartContract object, then a number of interaction transactions:

const smartContract = new SmartContract({ address: "..." });
const txA = smartContract.deploy({ ... });
const txB = smartContract.call({ ... });

Creating a RelayedTransaction(V1|V2)Builder, then craft a transaction:

const builder = new RelayedTransactionV1Builder();
const txA = builder
    .set...()
    ...
    .build();

References:

• https://docs.multiversx.com/sdk-and-tools/sdk-js/sdk-js-cookbook/#token-transfers
• https://github.com/multiversx/mx-sdk-js-core/blob/main/src/tokenOperations/tokenOperationsFactory.test.net.spec.ts
• https://docs.multiversx.com/sdk-and-tools/sdk-js/sdk-js-cookbook/#contract-interactions

Design points

1. All factories would receive a single constructor argument, which is a configuration object (to hold, among others, the chain ID and gas limit configuration) - see TokenOperationsFactory, as an example.
2. Public methods of a factory would return constructed transactions, ready-to-sign or almost ready-to-sign (depending on whether the user also passed the nonce when calling the method of the factory). See TokenOperationsFactory for an example.

Changes to make

• TransferTransactionsFactory - will not receive a GasEstimator as a constructor parameter anymore, but a factory config, instead.
• logic of RelayedTransactionV1Builder and RelayedTransactionV2Builder would be merged into a RelayedTransactionsFactory.
• logic of SmartContract - which does behave as a transactions factory (in part) would be extracted to a SmartContractTransactionsFactory.

Progressive roll-out

• The factories would ideally be added in v12, without breaking changes.
• The superseded components will be marked as deprecated, as the new ones are written and released.
• When name collision occurs, the new components will be prefixed with Next*. E.g. NextTransferTransactionsFactory.
• In next major version (e.g. v13), the superseded components will be removed, and the Next* prefix will be dropped.
• Eventual changes of the Interaction component will be handled separately (another proposal, another change set etc.)

Feedback

Let us know what you think 🙏

[michavie]

These are great improvements that will definitely improve the developer experience – consistency is always great!

Adding an idea for consideration with the goal to expose a clean & intuitive API for developers:

In the proposed state, developers will still have to remember various factory class names because of multiple points of entry for each factory type, increasing cognitive load. To improve things further, an idea is to add a parent class (a layer on top), named TransactionFactory, to have a single point of entry that exposes further sub-factories via its properties – all with a clean API for developers to work with.

This parent class can also abstract behavior of e.g. Relayed-Transactions, Guardians, etc for further simplicity on the developers end.

Examples:

TransactionFactory.transfers.createEGLDTransfer(...) // .transfers -> factories/transferFactory.ts
TransactionFactory.tokens.issueFungible(...) // .tokens -> factories/tokenFactory.ts
TransactionFactory.contracts.deploy(...) // .contracts -> factories/contractFactory.ts
...

Relaying a transaction could then proof as simple as chaining an optional method withRelayer:

TransactionFactory
    .withRelayer(...) // or withRelayer(V1|V2) (optional)
    .contracts
    .call(...)

By going this route, we also avoid introducing 'Next*' prefixes which would lead to breaking changes in the future.

The overall idea is to simplify the DX & provide a clean API for developers to work with.

[gfusee]

While uniformisation is a good (and needed) thing, using “factory” in a JS library might not be intuitive at all for beginner. I see “factories” more as a “Java” thing than a “JS” one.

Besides the intuitiveness there is a risk to have the library more verbose, that can deter non-multiversx dev to work with if they need to read a lot of documentation before being able to do basic stuff.

[andreibancioiu]

Will be handled here: https://github.com/multiversx/mx-sdk-specs

[popenta]

The transaction factories have been uniformized in the latest release (v13.