Introducing TransactionTask builder pattern for fine grained transaction control#536
Introducing TransactionTask builder pattern for fine grained transaction control#536joaquim-verges merged 9 commits intomainfrom
TransactionTask builder pattern for fine grained transaction control#536Conversation
src/core/classes/TransactionTask.ts
Outdated
|
|
||
| // ////////////// Actions //////////////// | ||
|
|
||
| public async encode(): Promise<string> { |
There was a problem hiding this comment.
Can we get methods off this to get all parts of tx data - this encode function gives you the data param, but would be nice to get everything including nonce, value, gasLimit, etc. all in one structure from a function (similar to how fractal needs it). This would actually let us create this generic contract encoder interface for every function call.
There was a problem hiding this comment.
Here's an example of what the structure looks like: https://buildthirdweb.slack.com/archives/C03GRV58Y13/p1658382917

And implementation in Go: https://buildthirdweb.slack.com/archives/C03GRV58Y13/p165844326355217

There was a problem hiding this comment.
But maybe most convenient is just to have a single function that returns the entire tx object, however ethers expects it, so people can do signing themselves as well (or at least for now to access the data).
There was a problem hiding this comment.
yeah that's interesting basically make this one encodeFunctionData() and add another encodeTransaction() or something like that
| return this.encoder.encode(this.functionName, this.args); | ||
| } | ||
|
|
||
| public async submit(): Promise<ContractTransaction> { |
There was a problem hiding this comment.
Should we add docs on this interface as well (like on submit vs execute)
There was a problem hiding this comment.
yep definitely, adding it now
src/core/classes/TransactionTask.ts
Outdated
| public args: any[] = []; | ||
| public overrides: CallOverrides | undefined; | ||
|
|
||
| constructor(contractWrapper: ContractWrapper<any>, functionName: string) { |
There was a problem hiding this comment.
Would it be beneficial to use/at least support the argument object pattern which tends to be common in JS vs the builder pattern. We had builder before with claim conditions and moved away from it - these two aren't as complex (this comment applies to both TransactionTaskBuilder and TransactionTask) but could be more convenient
There was a problem hiding this comment.
I see what you mean - I kinda gravitated towards a builder pattern mainly coz im used to it. Might not be the JS way. I'll play with an object as argument, prob less code too
src/contracts/signature-drop.ts
Outdated
| quantity, | ||
| checkERC20Allowance, | ||
| ); | ||
| return TransactionTask.builder(this.contractWrapper, "claim") |
There was a problem hiding this comment.
nitpick would be more concise with object params pattern instead of factory/builder and (imo but maybe wrong) more idiomatic for JS
There was a problem hiding this comment.
Also, instead of passing this.contractWrapper here, could we add a function called buildTransaction to contractWrapper that lets us do this.contractWrapper.buildTransaction with the exact same API we've been using before for sendTransaction
Beginning with just the drop claim functions, to give more granularity for the transaction, introduced a new API:
Eventually we'll re-use this
TransactionTaskobject for other transactions, exposing that same fine grained control