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.

Sign up for GitHub

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

Merge "legacy" transfer transactions factory in NextTransferTransactionsFactory #405

Merged
merged 3 commits into from
Mar 21, 2024
Merged

Merge "legacy" transfer transactions factory in NextTransferTransactionsFactory #405

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c1971bd
merge TransferTransactionsFactory in NextTransferTransactionsFactory
popenta Mar 19, 2024
5959080
fixes after review
popenta Mar 20, 2024
3b143d5
more fixes after review
popenta Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 37 additions & 8 deletions src/transactionsFactories/transferTransactionsFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,19 @@ interface IGasEstimator {

/**
* Use this class to create transactions for native token transfers (EGLD) or custom tokens transfers (ESDT/NTF/MetaESDT).
* This name is only temporary, the class will be renamed to `TransferTransactionsFactory`.
*/
export class TransferTransactionsFactory {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment (above) should be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

private readonly config?: IConfig;
private readonly dataArgsBuilder?: TokenTransfersDataBuilder;
private readonly tokenComputer?: ITokenComputer;
private readonly gasEstimator?: IGasEstimator;

/**
* Should be instantiated using `Config` and `TokenComputer`.
* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow rogue comment. Can be incorporated into the above one (to avoid any negative impact in some IDEs).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporated it in the above comment

constructor(options: IGasEstimator | { config: IConfig; tokenComputer: ITokenComputer }) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can add extra comment (documentation comment) to mention that the variant with gas estimator is legacy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (this.isGasEstimator(options)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we drop the check here, does ES lint complain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's typescript that complains. Or perhaps I misunderstood your question.

this.gasEstimator = options;
Expand All @@ -81,7 +86,7 @@ export class TransferTransactionsFactory {
if (this.gasEstimator === undefined) {
return false;
}
return this.isGasEstimator(this.gasEstimator);
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function can also be implemented with return this.gasEstimator !== undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Changed!

}

private ensureMembersAreDefined() {
Expand Down Expand Up @@ -133,7 +138,7 @@ export class TransferTransactionsFactory {
}

if (numberOfTransfers === 1) {
return this.createSingleESDTTransferDraft(options);
return this.createSingleESDTTransferTransaction(options);
}

const transferArgs = this.dataArgsBuilder!.buildArgsForMultiESDTNFTTransfer(
Expand All @@ -155,6 +160,10 @@ export class TransferTransactionsFactory {
}).build();
}

/**
* This is a legacy method. Can only be used if the class was instantiated using `GasEstimator`.
* Use {@link createTransactionForNativeTokenTransfer} instead.
*/
createEGLDTransfer(args: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

nonce?: INonce;
value: ITransactionValue;
Expand All @@ -166,7 +175,9 @@ export class TransferTransactionsFactory {
chainID: IChainID;
}) {
if (!this.isGasEstimatorDefined()) {
throw new Err("`gasEstimator` is not defined. Instantiate the factory using the gasEstimator.");
throw new Err(
"You are calling a legacy function to create an EGLD transfer transaction. If this is your intent, then instantiate the class using a `GasEstimator`. Or, instead, use the new, recommended `createTransactionForNativeTokenTransfer` method.",
);
}

const dataLength = args.data?.length() || 0;
Expand All @@ -184,6 +195,10 @@ export class TransferTransactionsFactory {
});
}

/**
* This is a legacy method. Can only be used if the class was instantiated using `GasEstimator`.
* Use {@link createTransactionForESDTTokenTransfer} instead.
*/
createESDTTransfer(args: {
tokenTransfer: ITokenTransfer;
nonce?: INonce;
Expand All @@ -194,7 +209,9 @@ export class TransferTransactionsFactory {
chainID: IChainID;
}) {
if (!this.isGasEstimatorDefined()) {
throw new Err("`gasEstimator` is not defined. Instantiate the factory using the gasEstimator.");
throw new Err(
"You are calling a legacy function to create an ESDT transfer transaction. If this is your intent, then instantiate the class using a `GasEstimator`. Or, instead, use the new, recommended `createTransactionForESDTTokenTransfer` method.",
);
}

const { argumentsString } = new ArgSerializer().valuesToString([
Expand All @@ -220,6 +237,10 @@ export class TransferTransactionsFactory {
});
}

/**
* This is a legacy method. Can only be used if the class was instantiated using `GasEstimator`.
* Use {@link createTransactionForESDTTokenTransfer} instead.
*/
createESDTNFTTransfer(args: {
tokenTransfer: ITokenTransfer;
nonce?: INonce;
Expand All @@ -230,7 +251,9 @@ export class TransferTransactionsFactory {
chainID: IChainID;
}) {
if (!this.isGasEstimatorDefined()) {
throw new Err("`gasEstimator` is not defined. Instantiate the factory using the gasEstimator.");
throw new Err(
"You are calling a legacy function to create an ESDTNFT transfer transaction. If this is your intent, then instantiate the class using a `GasEstimator`. Or, instead, use the new, recommended `createTransactionForESDTTokenTransfer` method.",
);
}

const { argumentsString } = new ArgSerializer().valuesToString([
Expand Down Expand Up @@ -260,6 +283,10 @@ export class TransferTransactionsFactory {
});
}

/**
* This is a legacy method. Can only be used if the class was instantiated using `GasEstimator`.
* Use {@link createTransactionForESDTTokenTransfer} instead.
*/
createMultiESDTNFTTransfer(args: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comments (as above).

tokenTransfers: ITokenTransfer[];
nonce?: INonce;
Expand All @@ -270,7 +297,9 @@ export class TransferTransactionsFactory {
chainID: IChainID;
}) {
if (!this.isGasEstimatorDefined()) {
throw new Err("`gasEstimator` is not defined. Instantiate the factory using the gasEstimator.");
throw new Err(
"You are calling a legacy function to create a MultiESDTNFT transfer transaction. If this is your intent, then instantiate the class using a `GasEstimator`. Or, instead, use the new, recommended `createTransactionForESDTTokenTransfer` method.",
);
}

const parts: TypedValue[] = [
Expand Down Expand Up @@ -310,7 +339,7 @@ export class TransferTransactionsFactory {
});
}

private createSingleESDTTransferDraft(options: {
private createSingleESDTTransferTransaction(options: {
sender: IAddress;
receiver: IAddress;
tokenTransfers: NextTokenTransfer[];
Expand Down
Loading