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.

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

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

Torres-ssf
Copy link
Contributor

@Torres-ssf Torres-ssf commented Jun 24, 2024

Breaking 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

  • The deployContract method would wait until the transaction was fully processed.
  • It returned a single Contract instance that was immediately usable.

New Behavior

  • The deployContract method now waits only until the transaction is submitted.
  • It returns an object containing:

How to Handle the Breaking Change

The contract instance will only be usable after the transaction finishes processing.

Old Code:

// Will resolve when the TX to deploy the contract is processed
const contract = await contractFactory.deployContract();

// Safe to use contract immediately
const res = await contract.functions.xyz().call();

New Code:

// Will resolve when the TX to deploy the contract is submitted
const { transactionResponse, contract } = await contractFactory.deployContract();

// Wait for the transaction to be fully processed
const deployTxResult = await transactionResponse.waitForResult();

// Now it's safe to use the contract
const res = await contract.functions.xyz().call();

Or, use the awaitExecution flag to automatically wait for the transaction to be processed:

// Will resolve when the TX to deploy the contract is processed
const { transactionResponse, contract } = await contractFactory.deployContract({ 
  awaitExecution: true 
});

// Safe to use contract immediately
const res = await contract.functions.xyz().call();

@Torres-ssf Torres-ssf added feat Issue is a feature p0 High priority labels Jun 24, 2024
@Torres-ssf Torres-ssf added this to the 0.x mainnet milestone Jun 24, 2024
@Torres-ssf Torres-ssf self-assigned this Jun 24, 2024
Copy link
Contributor

@danielbate danielbate left a 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

apps/docs/src/guide/contracts/deploying-contracts.md Outdated Show resolved Hide resolved
apps/docs/src/guide/contracts/deploying-contracts.md Outdated Show resolved Hide resolved
packages/contract/src/contract-factory.ts Outdated Show resolved Hide resolved
packages/contract/src/contract-factory.ts Outdated Show resolved Hide resolved
@Torres-ssf
Copy link
Contributor Author

Torres-ssf commented Jun 25, 2024

Do you think it's worthwhile using Typegen consistently in our documentation - over using ContractFactory (example)?

(This can be a separate issue - just posing the question)

@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?

@danielbate
Copy link
Contributor

@petertonysmith94 @Torres-ssf just weighing in that I would love to see us favouring Typegen consistently for fuel-gauge and snippets as it's essential for browser testing, and for users sake. I've added it as a requirement to #1820. Then we could have contract factory having it's own doc snippet.

@petertonysmith94
Copy link
Contributor

@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 ContractFactory would be worthwhile, but I reckon it should be isolate document 😄

@arboleya
Copy link
Member

@danielbate @petertonysmith94 I lost a bit of context here, but I'd say we should use the fuels CLI instead, which internally will use typegen anyway. This will close the cycle of dogfooding our stuff in our test suites as much as possible.

Copy link
Member

@arboleya arboleya left a 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.

apps/docs/src/guide/contracts/deploying-contracts.md Outdated Show resolved Hide resolved
apps/docs/src/guide/contracts/deploying-contracts.md Outdated Show resolved Hide resolved
packages/contract/src/contract-factory.ts Outdated Show resolved Hide resolved
@danielbate
Copy link
Contributor

I'd say we should use the fuels CLI instead, which internally will use typegen anyway

@arboleya agreed. Right now we are building with the fuels CLI, so like we do in a few places in fuel-gauge, I would like to see us utilising those factories rather than importing the ContractFactory.

nedsalk
nedsalk previously approved these changes Jun 27, 2024
Copy link
Contributor

@nedsalk nedsalk left a comment

Choose a reason for hiding this comment

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

lgtm

@Torres-ssf Torres-ssf requested a review from nedsalk June 27, 2024 13:51
Comment on lines +28 to +32
const { contract } = await contractFactory.deployContract({
awaitExecution: true,
...deployConfig,
});

Copy link
Member

@arboleya arboleya Jun 28, 2024

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

Copy link
Member

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.

Copy link
Member

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();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arboleya Using a secondary method was my initial approach: 5f6ffdd

@FuelLabs/sdk-ts How do you feel about this proposal?

// same as is
const { contract } = contractFactory.deployContract(config);

// new method
const { txResponse, contractId } = await contractFactory.deployContractForLater(config);

Copy link
Member

@arboleya arboleya Jun 28, 2024

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.

Copy link
Member

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.

Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.56%(+0.02%) 71.35%(-0.02%) 76.94%(+0.03%) 79.64%(+0.02%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/contract/src/contract-factory.ts 57.14%
(+6.12%)
46.15%
(+0%)
50%
(+12.5%)
57.14%
(+6.12%)
🔴 packages/contract/src/test-utils/launch-test-node.ts 97.5%
(+0.14%)
96.15%
(-0.27%)
100%
(+0%)
97.56%
(+0.13%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Issue is a feature p0 High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make deployContract Method a Non-Blocking Call
7 participants