-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
feat!: add a non-blocking call for deploy contract #2597
base: master
Are you sure you want to change the base?
Conversation
…abs/fuels-ts into st/feat/make-deploy-contract-async
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, couple comments
@petertonysmith94 Good question. Since both approaches are valid we should have examples for each one. But at the same time, I think that having 2 different approaches can be confusing for new users. What do you think? |
@petertonysmith94 @Torres-ssf just weighing in that I would love to see us favouring Typegen consistently for |
@Torres-ssf I think we need to be consistent using one or the other, I'd favour typegen as it provides the best experience for end users. As @danielbate said - I think a section on using a |
@danielbate @petertonysmith94 I lost a bit of context here, but I'd say we should use the |
packages/abi-typegen/test/fixtures/templates/contract/factory.hbs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some small [optional] nits; pick what you want.
Overall, I'm still thinking if this approach is intuitive enough.
@arboleya agreed. Right now we are building with the fuels CLI, so like we do in a few places in |
Co-authored-by: Anderson Arboleya <[email protected]>
Co-authored-by: Anderson Arboleya <[email protected]>
Co-authored-by: Anderson Arboleya <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
const { contract } = await contractFactory.deployContract({ | ||
awaitExecution: true, | ||
...deployConfig, | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered splitting the functionality?
I think it could bring some more cohesion and make it easier to follow.
// breaking change
const { txResponse, contractId } = await contractFactory.deployContract(config);
// new method
const { contract } = await contractFactory.deployAndAwaitForContract(config);
To avoid breaking changes, the opposite could be done:
// same as is
const { contract } = contractFactory.deployContract(config);
// new method
const { txResponse, contractId } = await contractFactory.deployContractForLater(config);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current approach, one must wait on a promise that doesn't contain the value they're waiting for.
The value (Contract
instance) is outside the promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For combining the Promise and the Contract
instance, another option could be:
// breaking change
const { contractId, waitForDeployment } = contractFactory.deployContract(config);
// new method
const { contract } = await waitForDeployment();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have the context of your first approach, sorry. I like the idea of a second method!
Even on the non-blocking route, we can offer a contract
instance at the end.
I just want the instance to come from the Promise I'm waiting for:
const { awaitForDeployment } = await contractFactory.deployForLater(config);
const { contract } = await awaitForDeployment();
We could also return the other properties, like txReponse
, contractId
etc.:
const {
txReponse,
contractId,
awaitForDeployment,
} = await contractFactory.deployForLater(config);
And it would still be non-breaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want the instance to come from the Promise I'm waiting for:
const { awaitForDeployment } = await contractFactory.deployForLater(config);
const { contract } = await awaitForDeployment();
I think there is credence to @arboleya suggestion because we would still be enabling non-blocking call, and it is also poka-yoked to prevent users from calling a contract instance that is not yet deployed.
In the current state of the PR, if I call factory.deployContract
with awaitExecution: false
, the contract
instance returned may throw an error that I will have to handle in my code. I think we should steer consumers away from that code path.
Coverage Report:
Changed Files:
|
deployContract
Method a Non-Blocking Call #2561Breaking Change
The deployContract method of the ContractFactory class has been updated to enhance its usability and efficiency. Previously, this method waited for the transaction to be fully processed before returning, providing only the Contract instance. This approach forced users to wait several seconds without any way to handle the transaction non-blocking.
Old Behavior
deployContract
method would wait until the transaction was fully processed.New Behavior
deployContract
method now waits only until the transaction is submitted.transactionResponse
: an instance of a TransactionResponse.contract
: A Contract instance.How to Handle the Breaking Change
The contract instance will only be usable after the transaction finishes processing.
Old Code:
New Code:
Or, use the
awaitExecution
flag to automatically wait for the transaction to be processed: