feat: dryRun and simulate will add output variables #549

QuinnLee · 2022-10-27T03:11:49Z

TLDR

using dryRun or simulate will add output variables.

github-actions · 2022-10-27T03:39:28Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟢	Statements	90.15% (+0.01% 🔼)	3579/3970
🟡	Branches	72.75% (+0.17% 🔼)	702/965
🟢	Functions	88.03% (+0.04% 🔼)	721/819
🟢	Lines	90.16% (+0% 🔼)	3428/3802

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / base-locked-wallet.ts	90.91% (-0.95% 🔻)	70%	93.33%	90.91% (-0.95% 🔻)

Test suite run success

542 tests passing in 49 suites.

Report generated by 🧪jest coverage report action from 0559a93

luizstacio · 2022-10-27T14:33:57Z

packages/fuel-gauge/src/token-test-contract.test.ts

@@ -53,4 +53,50 @@ describe('TokenTestContract', () => {
    const tokenBalance = balances.find((b) => b.assetId === token.id.toB256());
    expect(tokenBalance?.amount.toHex()).toEqual(toHex(50));
  });
+
+  it('Can add  variable estimations', async () => {


Suggested change

it('Can add variable estimations', async () => {

it('Automatically add variableOuputs', async () => {

packages/contract/src/contracts/functions/base-invocation-scope.ts

luizstacio · 2022-10-27T14:54:59Z

packages/contract/src/contracts/functions/base-invocation-scope.ts

@@ -184,6 +195,29 @@ export class BaseInvocationScope<TReturn = any> {
    return this;
  }

+  async addMissingVariableOutputs(


I think missing variables should happen in the same place we estimate gasFee for transactions on the provider level.

fuels-ts/packages/providers/src/provider.ts

Line 242 in 2cf77e8

const { gasUsed, minGasPrice } = await this.getTransactionCost(transactionRequest, 0);

And it should happen on dryRun and sendTransaction also. What do you think?

We can do this.

For clarification:
dryRun on the base-invocation-scope uses provider.call, I can add it to both provider.sendTransaction and provider.call functions.

yes, the issue I just thought about on the send transaction is that doing on the provider, will not be possible to change the tx, but we could return errors as we do with gas. And maybe an instruction on how to get this info first.

this is correct, signTransaction and populateTransactionWitnessesSignature are on the wallet

@luizstacio - I refactored provider.call to add the variables. But I'm having issues with provider.sendTransaction. The gas estimation + adding output variables results in test failures. My suggestion is to merge the new provider.call code in this PR and open another issue to add feature to provider.sendTransaction so this PR doesn't get too old and we can ship smaller chunks.

I think the idea on sendTransaction is to run the check for variable outputs on dry run mode with UTXO Validation false. If it doesn't have variable outputs just throw an error. If tests fail would be because of missing variable outputs. Otherwise, it should keep passing no?

camsjams

Looks good pending other changes

luizstacio · 2022-10-31T14:48:21Z

packages/providers/src/provider.ts

@@ -235,12 +245,17 @@ export default class Provider {
   * Submits a transaction to the chain to be executed
   */
  async sendTransaction(
-    transactionRequestLike: TransactionRequestLike
+    transactionRequestLike: TransactionRequestLike,
+    { signTransaction }: Partial<ProviderCallParams> = {}


I have some thoughts on this;

The signing process should take place before the tx arrives at the provider level.

getReceiptsWithMissingOutputVariables should always run on dryRun mode with utxValidation: off, avoiding signing a tx on its try. Also, imagining a scenario where the user needs to approve every try would make the UX bad.

packages/wallet/src/base-locked-wallet.ts

packages/transactions/src/coders/receipt.ts

packages/wallet/src/base-unlocked-wallet.ts

packages/providers/src/provider.ts

digorithm

Nice -- LGTM!

QuinnLee self-assigned this Oct 27, 2022

QuinnLee force-pushed the ql/530 branch 3 times, most recently from 419b9b7 to 73b9f12 Compare October 27, 2022 03:32

QuinnLee requested review from luizstacio, LuizAsFight, camsjams and a team October 27, 2022 12:29

luizstacio requested changes Oct 27, 2022

View reviewed changes

camsjams previously approved these changes Oct 27, 2022

View reviewed changes

QuinnLee dismissed camsjams’s stale review via 39163ab October 31, 2022 05:01

luizstacio requested changes Oct 31, 2022

View reviewed changes

QuinnLee force-pushed the ql/530 branch 4 times, most recently from 3345613 to b6f7b29 Compare October 31, 2022 16:18

luizstacio requested changes Oct 31, 2022

View reviewed changes

packages/wallet/src/base-locked-wallet.ts Show resolved Hide resolved

QuinnLee force-pushed the ql/530 branch from b6f7b29 to 8e53407 Compare October 31, 2022 16:31

feat: call & sendTransaction will add output variables

7cba801

QuinnLee force-pushed the ql/530 branch from 8e53407 to 7cba801 Compare October 31, 2022 16:33

camsjams suggested changes Oct 31, 2022

View reviewed changes

packages/transactions/src/coders/receipt.ts Outdated Show resolved Hide resolved

packages/wallet/src/base-unlocked-wallet.ts Show resolved Hide resolved

Pr comments

9e7d085

QuinnLee requested review from camsjams, luizstacio and a team October 31, 2022 18:39

QuinnLee enabled auto-merge (squash) October 31, 2022 21:20

lint errors

515e7f8

digorithm reviewed Oct 31, 2022

View reviewed changes

packages/providers/src/provider.ts Show resolved Hide resolved

camsjams previously approved these changes Oct 31, 2022

View reviewed changes

update comments

e56f4fa

QuinnLee dismissed camsjams’s stale review via e56f4fa November 1, 2022 03:42

Merge branch 'master' into ql/530

0559a93

QuinnLee requested review from digorithm, camsjams and a team November 1, 2022 03:43

digorithm approved these changes Nov 1, 2022

View reviewed changes

camsjams approved these changes Nov 1, 2022

View reviewed changes

luizstacio approved these changes Nov 1, 2022

View reviewed changes

QuinnLee merged commit db8cc6b into master Nov 1, 2022

QuinnLee deleted the ql/530 branch November 1, 2022 13:23

github-actions bot mentioned this pull request Nov 1, 2022

ci(changesets): versioning packages #553

Merged

QuinnLee linked an issue Nov 2, 2022 that may be closed by this pull request

Add variables outputs automatically by simulating tx #530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dryRun and simulate will add output variables #549

feat: dryRun and simulate will add output variables #549

QuinnLee commented Oct 27, 2022 •

edited

Loading

github-actions bot commented Oct 27, 2022 •

edited

Loading

luizstacio Oct 27, 2022

luizstacio Oct 27, 2022

QuinnLee Oct 27, 2022

luizstacio Oct 27, 2022 •

edited

Loading

QuinnLee Oct 27, 2022

QuinnLee Oct 31, 2022

luizstacio Oct 31, 2022

camsjams left a comment

luizstacio Oct 31, 2022

digorithm left a comment

	it('Can add variable estimations', async () => {
	it('Automatically add variableOuputs', async () => {

feat: dryRun and simulate will add output variables #549

feat: dryRun and simulate will add output variables #549

Conversation

QuinnLee commented Oct 27, 2022 • edited Loading

TLDR

github-actions bot commented Oct 27, 2022 • edited Loading

Coverage report

Test suite run success

luizstacio Oct 27, 2022

Choose a reason for hiding this comment

luizstacio Oct 27, 2022

Choose a reason for hiding this comment

QuinnLee Oct 27, 2022

Choose a reason for hiding this comment

luizstacio Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

QuinnLee Oct 27, 2022

Choose a reason for hiding this comment

QuinnLee Oct 31, 2022

Choose a reason for hiding this comment

luizstacio Oct 31, 2022

Choose a reason for hiding this comment

camsjams left a comment

Choose a reason for hiding this comment

luizstacio Oct 31, 2022

Choose a reason for hiding this comment

digorithm left a comment

Choose a reason for hiding this comment

QuinnLee commented Oct 27, 2022 •

edited

Loading

github-actions bot commented Oct 27, 2022 •

edited

Loading

luizstacio Oct 27, 2022 •

edited

Loading