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

Optimize estimateTxDependencies method #1763

Open
Torres-ssf opened this issue Feb 15, 2024 · 8 comments
Open

Optimize estimateTxDependencies method #1763

Torres-ssf opened this issue Feb 15, 2024 · 8 comments
Labels
chore Issue is a chore

Comments

@Torres-ssf
Copy link
Contributor

Torres-ssf commented Feb 15, 2024

The estimateTxDependencies automates the addition of missing OutputVariables, essential for Sway's transfer or mint functions.

With this, users don't need to specify the variableOutputs parameter:

const { transactionResult } = await contract
  .functions
  .mint_to_addresses(address, subId, mintAmount)
  .txParams({
    variableOutputs: 3, // <—————————— this
  })
  .call();

Warning

While providing a nice DX, this approach may lead to multiple dry-run calls, potentially slowing down the transaction execution and representing more use of resources, which translates into more costs.

How it works:

  1. The method performs dry runs, checking output receipts for a Revert with val equal to TransferToAddressFailed. This error indicates a missing OutputVariable.
  2. If such a Revert receipt is detected, an OutputVariable is added, and another dry run is executed. This loop continues until no related revert receipt is found.

Consider a contract call minting assets to 3 addresses:

    fn mint_to_addresses(addresses: [Address; 3], sub_id: b256, mint_amount: u64) {
        assert(sub_id == TOKEN_1 || sub_id == TOKEN_2 || sub_id == TOKEN_3);
        let mut counter = 0;
        while counter < 3 {
            mint_to_address(addresses[counter], sub_id, mint_amount);
            counter = counter + 1;
        }
    }

Outcome:

  1. Since this function mints a coin to 3x addresses, it will need:
    • 3x OutputVariable
  2. This means the dry-run call will fail:
    • 3x times
  3. And succeed only in the:
    • 4th execution
  4. This will result in:
    • 4x total dry-runs

Proposed change:

  • Discontinue automatic addition of OutputVariable
    • Shift this responsibility to users
    • Save up resources
  • Docs
    • Document how to use OutputVariable in the SDK documentation.
    • Include information about the error returned when an OutputVariable is missing

Related:

@Torres-ssf Torres-ssf added chore Issue is a chore mainnet labels Feb 15, 2024
@Torres-ssf Torres-ssf assigned Torres-ssf and unassigned Torres-ssf Feb 15, 2024
@nedsalk
Copy link
Contributor

nedsalk commented Feb 15, 2024

@Dhaiwat10 Dhaiwat10 self-assigned this Mar 4, 2024
@Dhaiwat10
Copy link
Member

Does this still need to be fixed after #1767 ?

@Torres-ssf
Copy link
Contributor Author

@Dhaiwat10 If I am not mistaken, for this one we are waiting for a possible update on fuel-core that will include on the dryRun response the amount of OutputVariables required by a TX when calling dryRun.

@arboleya
Copy link
Member

arboleya commented Mar 4, 2024

@Torres-ssf Did we communicate this need to someone?

Ideally, we should mark this issue as blocked and add a link here to another issue on the fuel-core repo.

@Torres-ssf
Copy link
Contributor Author

@arboleya @Dhaiwat10 I reckon this issue is blocked by FuelLabs/fuel-vm#691

It seems the plan to implement this is only after mainnet.

@arboleya
Copy link
Member

arboleya commented Mar 6, 2024

That was the missing issue, then.

Thanks for pointing that out.

@nedsalk
Copy link
Contributor

nedsalk commented Mar 6, 2024

Based on the issue's proposed changes (specifically "Discontinue automatic addition of OutputVariable"), this doesn't seem like blocked. This issue is concerned with not running transaction dependency estimation by default anymore, whereas the linked fuel-vm issue - if I understood it correctly - is concerned with getting all the dependencies in one call instead of requiring multiple calls:

The FuelVM may support dry run mode, in which all validity rules will not abort the execution immediately. Instead, we will keep a record of what rules are failing and provide feedback at the end of the execution with all problems.

We can already make dependency estimation and opt-in thing, it'd just break the apps of our consumers that didn't specify the appropriate outputs.

@Dhaiwat10 Dhaiwat10 removed their assignment Mar 6, 2024
@arboleya arboleya added the p0 label Jun 9, 2024
@arboleya arboleya added this to the Mainnet milestone Jun 9, 2024
@arboleya arboleya added p1 and removed mainnet labels Jun 9, 2024
@arboleya arboleya modified the milestones: 0.x mainnet, 0.x post-launch Jun 12, 2024
@arboleya arboleya added p2 and removed p1 labels Jun 12, 2024
@arboleya arboleya removed this from the 0.x post-launch milestone Jul 19, 2024
@arboleya arboleya removed the blocked label Jul 19, 2024
@Torres-ssf
Copy link
Contributor Author

@nedsalk This is blocked.

The desired behavior, as outlined in this issue, is for the dry-run to indicate the number of variable outputs required for the transaction to be processed.

Implementing this feature will eliminate the need for multiple dry-runs to determine the necessary outputs.

We cannot expect users to always provide the correct number of variable outputs, especially for complex transactions involving multiple parties ( wallet connectors ).

@arboleya arboleya removed the p2 label Aug 1, 2024
@arboleya arboleya removed the blocked label Sep 7, 2024
@arboleya arboleya added the temp:notion label Sep 8, 2024 — with Linear
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issue is a chore
Projects
None yet
Development

No branches or pull requests

4 participants