Skip to content
This repository was archived by the owner on Aug 30, 2022. It is now read-only.

Introducing TransactionTask builder pattern for fine grained transaction control#536

Merged
joaquim-verges merged 9 commits intomainfrom
joaquim/tx_object_api
Aug 7, 2022
Merged

Introducing TransactionTask builder pattern for fine grained transaction control#536
joaquim-verges merged 9 commits intomainfrom
joaquim/tx_object_api

Conversation

@joaquim-verges
Copy link
Copy Markdown
Member

@joaquim-verges joaquim-verges commented Aug 5, 2022

Beginning with just the drop claim functions, to give more granularity for the transaction, introduced a new API:

const tx = dropContract.getClaimTransaction(receiver, quantity);
// you can override stuff
tx.overrideGasLimit(...)
tx.overrideNonce(...)
// you can run estimations
tx.estimateGasCostInEther(...)
// you can encode tx data
tx.encode()
// you can submit without waiting for confirmations
tx.submit()
// you can execute (what we do today)
tx.execute()

Eventually we'll re-use this TransactionTask object for other transactions, exposing that same fine grained control


// ////////////// Actions ////////////////

public async encode(): Promise<string> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add docs on this interface as well (like on submit vs execute)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yep definitely, adding it now

public args: any[] = [];
public overrides: CallOverrides | undefined;

constructor(contractWrapper: ContractWrapper<any>, functionName: string) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

quantity,
checkERC20Allowance,
);
return TransactionTask.builder(this.contractWrapper, "claim")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick would be more concise with object params pattern instead of factory/builder and (imo but maybe wrong) more idiomatic for JS

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@joaquim-verges joaquim-verges merged commit 4f63349 into main Aug 7, 2022
@joaquim-verges joaquim-verges deleted the joaquim/tx_object_api branch August 7, 2022 04:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants