added a builder for relayed v2 transactions #221
Conversation
src/relayedTransactionV2Builder.ts
Outdated
| receiver: innerTransaction.getSender(), | ||
| value: 0, | ||
| gasLimit: | ||
| innerTransactionGasLimit.valueOf() + networkConfig.MinGasLimit + networkConfig.GasPerDataByte * payload.length(), |
There was a problem hiding this comment.
Could we move this logic in https://github.com/ElrondNetwork/elrond-sdk-erdjs/blob/main/src/gasEstimator.ts ?
There was a problem hiding this comment.
I wouldn't add an extra dependency to this class. Also, it is pretty specific to this use-case
There was a problem hiding this comment.
That's exactly what gasEstimator does. Computes gas for every specific use-case.
I would keep the consistency, we could also ask @andreibancioiu 's opinion on this
There was a problem hiding this comment.
(added a separate comment)
Both options are valid and both are backed by good arguments ⚖️
| import {ISigner} from "@elrondnetwork/erdjs-walletcore/out/interface"; | ||
| import {INetworkConfig} from "./interfaceOfNetwork"; | ||
|
|
||
| export class RelayedTransactionV2Builder { |
There was a problem hiding this comment.
Allt tx builders in this repo are defined here: https://github.com/ElrondNetwork/elrond-sdk-erdjs/blob/main/src/transactionFactory.ts. Could we move the logic here to keep consistency?
There was a problem hiding this comment.
it is rather more similar to tokenTransferBuilders.ts so I will leave it as it is
There was a problem hiding this comment.
tokenTransferBuilders builds the payLoad. transactionFactory builds the txs. You could've split relayedTransactionV2Builder in 3 to keep consistency:
- gasEstimator for relayed tx
- payloadBuilder for relayed tx
- txFactory to create the relayed tx.
Again, let's also hear @andreibancioiu 's opinion
There was a problem hiding this comment.
The need for consistency is indeed a very good argument. On the other hand, this new component is self-contained, which is also good.
If we were to move gasEstimator.ts, tokenTransferBuilders.ts and transactionFactory.ts to a new package called transfers (in the future), then this design is a good choice. It's possible that in the future we might even extract the following logic outside of erdjs (and move it to separate packages):
- transfers (EGLD and all kinds of ESDT)
- relayed transactions (V1 & V2)
- staking & delegation SC calls (actually, missing here now)
- esdt management calls (actually, missing here now)
So, no strong opinion at this moment (can stay as it is if we desire to further split erdjs functionality to separate NPM packages).
| chainID: networkConfig.ChainID, | ||
| data: new TransactionPayload("getContractConfig"), | ||
| }); | ||
| builder = builder.setNetworkConfig(networkConfig).setInnerTransactionGasLimit(10).setInnerTransaction(innerTx); |
| await bob.signer.sign(innerTx); | ||
|
|
||
| const builder = new RelayedTransactionV2Builder(); | ||
| const relayedTxV2 = builder |
| import {ISigner} from "@elrondnetwork/erdjs-walletcore/out/interface"; | ||
| import {INetworkConfig} from "./interfaceOfNetwork"; | ||
|
|
||
| export class RelayedTransactionV2Builder { |
There was a problem hiding this comment.
The need for consistency is indeed a very good argument. On the other hand, this new component is self-contained, which is also good.
If we were to move gasEstimator.ts, tokenTransferBuilders.ts and transactionFactory.ts to a new package called transfers (in the future), then this design is a good choice. It's possible that in the future we might even extract the following logic outside of erdjs (and move it to separate packages):
- transfers (EGLD and all kinds of ESDT)
- relayed transactions (V1 & V2)
- staking & delegation SC calls (actually, missing here now)
- esdt management calls (actually, missing here now)
So, no strong opinion at this moment (can stay as it is if we desire to further split erdjs functionality to separate NPM packages).
src/relayedTransactionV2Builder.ts
Outdated
| if(!this.innerTransaction || !this.innerTransactionGasLimit || !this.netConfig || !this.innerTransaction.getSignature()) { | ||
| throw new ErrInvalidRelayedV2BuilderArguments(); | ||
| } | ||
| if(this.innerTransaction.getGasLimit() !== 0){ |
There was a problem hiding this comment.
Comparison might fail if getGasLimit returns an object that implements IGasLimit. Perhaps allow type coercion to happen? != / or add a test with both numbers and objects if this works as it is.
| const innerTx = new Transaction({ | ||
| nonce: 15, | ||
| receiver: Address.fromBech32("erd1qqqqqqqqqqqqqqqpqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqzllls8a5w6u"), | ||
| gasLimit: 10, |
There was a problem hiding this comment.
Using some implementation of IGasLimit, such as { valueOf: function() { return 10; } } might fail the test.
There was a problem hiding this comment.
extended the test to include this as well
added a builder for relayed v2 txs.
tested on testnet by using the resulted transaction from the unit test:
https://testnet-explorer.elrond.com/transactions/c02398ab9f02e564539ef1d8acd0c36cb4b8720d4c3397ef2e2ed656177471ff