Merge "legacy" transfer transactions factory in NextTransferTransactionsFactory #405
Conversation
| @@ -3,7 +3,7 @@ import { Address } from "./address"; | |||
| import { GasEstimator } from "./gasEstimator"; | |||
| import { TokenTransfer } from "./tokenTransfer"; | |||
| import { TransactionPayload } from "./transactionPayload"; | |||
| import { TransferTransactionsFactory } from "./transferTransactionsFactory"; | |||
| import { TransferTransactionsFactory } from "./transactionsFactories/transferTransactionsFactory"; | |||
There was a problem hiding this comment.
Would lead to an eventual, small, breaking change (depending on how import statement is written in client code).
| this.config = options.config; | ||
| this.tokenComputer = options.tokenComputer; | ||
| this.dataArgsBuilder = new TokenTransfersDataBuilder(); | ||
| export class TransferTransactionsFactory { |
There was a problem hiding this comment.
Comment (above) should be updated.
| private readonly tokenComputer?: ITokenComputer; | ||
| private readonly gasEstimator?: IGasEstimator; | ||
|
|
||
| constructor(options: IGasEstimator | { config: IConfig; tokenComputer: ITokenComputer }) { |
There was a problem hiding this comment.
Can add extra comment (documentation comment) to mention that the variant with gas estimator is legacy.
| private readonly gasEstimator?: IGasEstimator; | ||
|
|
||
| constructor(options: IGasEstimator | { config: IConfig; tokenComputer: ITokenComputer }) { | ||
| if (this.isGasEstimator(options)) { |
There was a problem hiding this comment.
If we drop the check here, does ES lint complain?
There was a problem hiding this comment.
It's typescript that complains. Or perhaps I misunderstood your question.
| } | ||
| } | ||
|
|
||
| private isGasEstimator(options: any): options is IGasEstimator { |
There was a problem hiding this comment.
Interesting (hard-core TypeScript).
| if (this.gasEstimator === undefined) { | ||
| return false; | ||
| } | ||
| return this.isGasEstimator(this.gasEstimator); |
There was a problem hiding this comment.
Maybe this.gasEstimator === undefined is a "sufficient" check (covers the flow where the user called the constructor with gas estimator)?
There was a problem hiding this comment.
That should be enough. Removed.
| @@ -89,27 +155,184 @@ export class NextTransferTransactionsFactory { | |||
| }).build(); | |||
| } | |||
|
|
|||
| createEGLDTransfer(args: { | |||
There was a problem hiding this comment.
Add comments that they are legacy functions? Since old constructor is not deprecated, it's OK not to deprecate these, as well, but at least mark them with "This is a legacy function, use ... instead".
| chainID: IChainID; | ||
| }) { | ||
| if (!this.isGasEstimatorDefined()) { | ||
| throw new Err("`gasEstimator` is not defined. Instantiate the factory using the gasEstimator."); |
There was a problem hiding this comment.
Alter the message perhaps to recommend the new functions, instead.
"It seems that you are calling the legacy functions to create a transfer transaction. If this is your intent, then pass a gas estimator in the constructor. Or, instead, use the new, recommended ...".
There was a problem hiding this comment.
Done. Added for all legacy methods.
| }); | ||
| } | ||
|
|
||
| createMultiESDTNFTTransfer(args: { |
There was a problem hiding this comment.
Add comments (as above).
| chainID: IChainID; | ||
| }) { | ||
| if (!this.isGasEstimatorDefined()) { | ||
| throw new Err("`gasEstimator` is not defined. Instantiate the factory using the gasEstimator."); |
There was a problem hiding this comment.
Adjust message, as above (to point user to the new way).
| * Instantiating this class using GasEstimator represents the legacy version of this class. | ||
| * The legacy version contains methods like `createEGLDTransfer`, `createESDTTransfer`, `createESDTNFTTransfer` and `createMultiESDTNFTTransfer`. | ||
| */ | ||
| // this was done to minimize breaking changes in client code |
There was a problem hiding this comment.
Somehow rogue comment. Can be incorporated into the above one (to avoid any negative impact in some IDEs).
There was a problem hiding this comment.
Incorporated it in the above comment
| @@ -81,7 +86,7 @@ export class TransferTransactionsFactory { | |||
| if (this.gasEstimator === undefined) { | |||
| return false; | |||
| } | |||
| return this.isGasEstimator(this.gasEstimator); | |||
| return true; | |||
There was a problem hiding this comment.
Function can also be implemented with return this.gasEstimator !== undefined.
Merged "legacy" transfer transactions factory in
NextTransferTransactionsFactoryand renamedNextTransferTransactionsFactorytoTransferTransactionsFactory.