refactor: rename `addMissingVariables` to `estimateTxDependencies`, and add docs for tx dep estimation #924

Dhaiwat10 · 2023-04-24T10:18:12Z

Summary

As discussed here, this PR renames the addMissingVariables helper to estimateTxDependencies for better consistency with the Rust SDK, and adds a docs page for transaction dependency estimation.

I have based this PR off of @Torres-ssf's docs snippet branch since it contains the updated guides around contracts. This PR should automatically change base to master once his PR is merged.

Closes #701

…page

…ppets-contracts

Dhaiwat10 · 2023-04-24T10:22:10Z

apps/docs/src/guide/contracts/transaction-dependency-estimation.md

+However, the SDK always automatically estimates these dependencies and double-checks if everything is in order whenever you invoke a contract function or attempt to send a transaction.
+


@digorithm I believe that this behaviour here in the TS SDK is slightly different from the Rust SDK. The Rust SDK doesn't seem to automatically call estimate_tx_dependencies on each function invocation, while the TS SDK does that every single time automatically.

I feel like this would make the estimateTxDependencies function here in the TS SDK more of an internal helper since SDK users will never have to call it manually.

Yeah. I'd suggest that, in the near future, we should move to manually calling estimateTxDependencies; the additional dry-run calls happening without the user's consent could be an undesired behavior. Or it could be an optional value we set at the call time. We can discuss that later.

github-actions · 2023-04-24T10:25:12Z

Coverage report

❌ An unexpected error occurred. For more details, check console

Error: The process '/home/runner/setup-pnpm/node_modules/.bin/pnpm' failed with exit code 1

St.^❔	Category	Percentage	Covered / Total
🟢	Statements	95.09% (+0.01% 🔼)	4702/4945
🟢	Branches	83.97% (-0.04% 🔻)	833/992
🟢	Functions	92.22% (-0.15% 🔻)	901/977
🟢	Lines	95.11% (+0.01% 🔼)	4495/4726

⚠️ Details were not displayed: the report size has exceeded the limit.

Test suite run failed

Failed tests: 1/822. Failed suites: 1/126.

  ● ExampleContract › should return the input

    Transaction id was already used: 0xeab9f726ba65507def084bc3bdd9107c32ea6793f602c3411975f175eee5b5f8: {"response":{"data":null,"errors":[{"message":"Transaction id was already used: 0xeab9f726ba65507def084bc3bdd9107c32ea6793f602c3411975f175eee5b5f8","locations":[{"line":2,"column":3}],"path":["dryRun"]}],"status":200,"headers":{}},"request":{"query":"mutation dryRun($encodedTransaction: HexString!, $utxoValidation: Boolean) {\n  dryRun(tx: $encodedTransaction, utxoValidation: $utxoValidation) {\n    ...receiptFragment\n  }\n}\n\nfragment receiptFragment on Receipt {\n  data\n  rawPayload\n}","variables":{"encodedTransaction":"0x000000000000000000000000000000000000000005f5e1000000000000000000000000000000000400000000000000000000000000000001000000000000000200000000000000010000000000000000000000000000000000000000000000000000000000000000240000000000000000000000000000006b00bb793fec81293cbe8d9d6f180e0ef9a5a87e5e2d47a68394cb7e82ebeaeb000000000000000009c0b2d1a486c439a87bcba6b46a7a1a23f3897cc83a94521a96da5c23bc58dbffffffffbaf3c2010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000309c0b2d1a486c439a87bcba6b46a7a1a23f3897cc83a94521a96da5c23bc58db0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003371ceecdb6f9b8c308ef075845defc4eec5c2056d89aec6d26ad00f47e6564400000000000003e800000000000000000000000000000000000000000000000000000000000000000000000000000040eddac5913738e59f33196df00d3138d480ae28cf740d4e31754e348fb1c673b5ba3cc75ac6eda99f7ba9c1df9e495c034ec91541f4bc6ef869519cc8201dc41d","utxoValidation":false}}}

      at node_modules/.pnpm/[email protected][email protected]/node_modules/graphql-request/src/index.ts:497:11
      at step (node_modules/.pnpm/[email protected][email protected]/node_modules/graphql-request/dist/index.js:63:23)
      at Object.next (node_modules/.pnpm/[email protected][email protected]/node_modules/graphql-request/dist/index.js:44:53)
      at fulfilled (node_modules/.pnpm/[email protected][email protected]/node_modules/graphql-request/dist/index.js:35:58)

Report generated by 🧪jest coverage report action from 67653fe

apps/docs/src/guide/contracts/transaction-dependency-estimation.md

Dhaiwat10 · 2023-04-24T10:30:43Z

packages/providers/src/provider.ts

+   *
+   * If there are missing output contract IDs, those are fetched and added to the transaction.
+   *
+   * TODO: Add support for missing output messages


Will file a follow-up issue for this.

camsjams

…ppets-contracts

…ts' into dp/estimate-tx-deps

Torres-ssf

Nice job @Dhaiwat10 👍.

But I have one question though

Torres-ssf · 2023-04-25T11:54:29Z

packages/providers/src/provider.ts

+   *
+   * If there are missing output contract IDs, those are fetched and added to the transaction.
+   *


Is this functionality working? When I was rewriting the Inter-contract Calls doc page I was always running into an error before explicitly calling the addContracts function in the contract function chain call.

@Torres-ssf there seems to be code in place that would take care of that functionality, but we don't have any tests in place for this.

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

Lines 374 to 376 in dadc576

missingOutputContractIds.forEach(({ contractId }) =>

transactionRequest.addContract(Address.fromString(contractId))

);

@Dhaiwat10 I've just tried to validate that and it seems this is not working or at least for not all cases.

It seems this functionality relies on the following logic that is defined here.

receipt.type === ReceiptType.Panic && receipt.contractId !== '0x0000000000000000000000000000000000000000000000000000000000000000';

It seems the getReceiptsWithMissingData function loops through all receipts returned from a dryRun call and tries to verify if the receipt type and contractId properties indicate that a contract ID is missing in order to a successful script call.

However, in my tests, when I try to execute a contract call that does an inter-contract call, the receipt does not come with a contractId different than the one specified in the condition, thus, not being a valid case for missing contract ID.

{ type: 3, id: '0x48ca0552db61b135a3731928ce77910c3d3630a554ef313949d6af14930177ba', reason: <BN: 0x172d4d2450000000>, pc: <BN: 0x481c>, is: <BN: 0x4338>, contractId: '0x0000000000000000000000000000000000000000000000000000000000000000' },

I couldn't find out the source of the problem, maybe it could be something in the dry-run call?

Do you have any insight on that?

@Torres-ssf thanks for looking into this with so much detail. Unfortunately, I don't have a lot of insight on the dry-run implementation, but this is def something we need to fix. I can file a follow-up issue for this. Does that sound good?

@Dhaiwat10 Yes that sounds great

arboleya · 2023-04-28T13:14:24Z

Just a side note — there are dozens of unrelated commits here. I'd guess this PR wasn't based on the master branch.

Dhaiwat10 · 2023-04-28T19:37:18Z

@arboleya good heavens. I based this PR off of the branch for #895 since I wanted to use some of the work from there. This has blown up 😆

Dhaiwat10 · 2023-04-28T20:14:49Z

Closing this in favor of #954. Tried rebasing this PR against master but there are way too many merge conflicts. Creating a new PR seems to be the wiser choice.

Torres-ssf added 30 commits April 11, 2023 09:50

create package for code snippets

15297bc

create contract echo-values for snippets package

fb10948

properly setup snippets contracts dir

4f394be

add utility to load ABI and BIN files for snippet contracts

bd5fc4f

add function to create test wallet for snippets package

07dda7b

implement snippet test file for contracts index page

b2c8bbe

implement snippet test file for contracts interacting-with-contracts …

8f861d0

…page

update content of doc page interacting-with-contracts

0745ea3

implement snippet test file for contracts read-only-calls page

a19db5c

update content of doc page read-only-calls

b335135

implement snippet test file for contracts deploying-contracts page

920b646

update content of doc page deploying-contracts

2a7740d

change deploying-contracts page order

631187e

refact test suite

4382f5c

create contract counter on snippets package

84eff32

define code snippet region

a5a55f3

implement snippet test file for contracts transaction-parameters page

0b89d6f

update content of doc page transaction-parameters

077a365

change transaction-parameters page order

0e7656f

make regex cleaner

b8f0303

create contract return context on snippets package

16d4d48

implement snippet test file for contracts call-parameters page

30ce753

update content of doc page call-parameters

d9ca919

change doc call-parameters page order

de7fc79

implement snippet test file for contracts multicalls page

e5030c2

update content of doc page multicalls

6dcf28c

Merge branch 'master' into torres/docs/add-dedicated-package-code-sni…

c523274

…ppets-contracts

renaming package snippets to doc-snippets

dc75e2a

implement snippet test file for contracts cost-estimation page

c436496

update content of doc page cost-estimation

b0e7113

Dhaiwat10 changed the base branch from master to torres/docs/add-dedicated-package-code-snippets-contracts April 24, 2023 10:18

Dhaiwat10 changed the title ~~refactor/docs: rename addMissingVariables to estimateTxDependencies, and add docs for tx dep estimation~~ refactor: rename addMissingVariables to estimateTxDependencies, and add docs for tx dep estimation Apr 24, 2023

Dhaiwat10 commented Apr 24, 2023

View reviewed changes

Dhaiwat10 mentioned this pull request Apr 24, 2023

docs: add dedicated package for code snippets from contracts section #895

Merged

Dhaiwat10 closed this Apr 24, 2023

Dhaiwat10 reopened this Apr 24, 2023

danielbate reviewed Apr 24, 2023

View reviewed changes

apps/docs/src/guide/contracts/transaction-dependency-estimation.md Outdated Show resolved Hide resolved

add a link for better context

50f0837

Dhaiwat10 requested review from arboleya, camsjams, digorithm, luizstacio, LuizAsFight, danielbate and Torres-ssf April 24, 2023 10:29

Dhaiwat10 self-assigned this Apr 24, 2023

Dhaiwat10 commented Apr 24, 2023

View reviewed changes

camsjams approved these changes Apr 24, 2023

View reviewed changes

arboleya added 2 commits April 24, 2023 21:38

Merge branch 'master' into torres/docs/add-dedicated-package-code-sni…

ee46b7c

…ppets-contracts

Merge branch 'torres/docs/add-dedicated-package-code-snippets-contrac…

67653fe

…ts' into dp/estimate-tx-deps

Base automatically changed from torres/docs/add-dedicated-package-code-snippets-contracts to master April 25, 2023 11:01

Torres-ssf requested changes Apr 25, 2023

View reviewed changes

Dhaiwat10 mentioned this pull request Apr 28, 2023

refactor: rename addMissingVariables to estimateTxDependencies, add docs #954

Merged

Dhaiwat10 closed this Apr 28, 2023

Dhaiwat10 mentioned this pull request Apr 28, 2023

Investigate support for missing contract IDs in estimateTxDependencies #956

Closed

Dhaiwat10 deleted the dp/estimate-tx-deps branch April 28, 2023 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: rename `addMissingVariables` to `estimateTxDependencies`, and add docs for tx dep estimation #924

refactor: rename `addMissingVariables` to `estimateTxDependencies`, and add docs for tx dep estimation #924

Dhaiwat10 commented Apr 24, 2023 •

edited

Loading

Dhaiwat10 Apr 24, 2023

digorithm Apr 24, 2023

github-actions bot commented Apr 24, 2023 •

edited

Loading

Dhaiwat10 Apr 24, 2023

camsjams left a comment

Torres-ssf left a comment

Torres-ssf Apr 25, 2023 •

edited

Loading

Dhaiwat10 Apr 25, 2023

Torres-ssf Apr 28, 2023

Dhaiwat10 Apr 28, 2023

Torres-ssf Apr 28, 2023

arboleya commented Apr 28, 2023

Dhaiwat10 commented Apr 28, 2023

Dhaiwat10 commented Apr 28, 2023

		However, the SDK always automatically estimates these dependencies and double-checks if everything is in order whenever you invoke a contract function or attempt to send a transaction.

	missingOutputContractIds.forEach(({ contractId }) =>
	transactionRequest.addContract(Address.fromString(contractId))
	);

refactor: rename addMissingVariables to estimateTxDependencies, and add docs for tx dep estimation #924

refactor: rename addMissingVariables to estimateTxDependencies, and add docs for tx dep estimation #924

Conversation

Dhaiwat10 commented Apr 24, 2023 • edited Loading

Summary

Dhaiwat10 Apr 24, 2023

Choose a reason for hiding this comment

digorithm Apr 24, 2023

Choose a reason for hiding this comment

github-actions bot commented Apr 24, 2023 • edited Loading

Coverage report

Test suite run failed

Dhaiwat10 Apr 24, 2023

Choose a reason for hiding this comment

camsjams left a comment

Choose a reason for hiding this comment

Torres-ssf left a comment

Choose a reason for hiding this comment

Torres-ssf Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Dhaiwat10 Apr 25, 2023

Choose a reason for hiding this comment

Torres-ssf Apr 28, 2023

Choose a reason for hiding this comment

Dhaiwat10 Apr 28, 2023

Choose a reason for hiding this comment

Torres-ssf Apr 28, 2023

Choose a reason for hiding this comment

arboleya commented Apr 28, 2023

Dhaiwat10 commented Apr 28, 2023

Dhaiwat10 commented Apr 28, 2023

refactor: rename `addMissingVariables` to `estimateTxDependencies`, and add docs for tx dep estimation #924

refactor: rename `addMissingVariables` to `estimateTxDependencies`, and add docs for tx dep estimation #924

Dhaiwat10 commented Apr 24, 2023 •

edited

Loading

github-actions bot commented Apr 24, 2023 •

edited

Loading

Torres-ssf Apr 25, 2023 •

edited

Loading