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

Add message output estimation #846

Merged
merged 7 commits into from
Feb 27, 2023
Merged

Conversation

MujkicA
Copy link
Contributor

@MujkicA MujkicA commented Feb 16, 2023

Closes #845

Extends estimate_tx_dependancies to also add output messages if they're missing.

@MujkicA MujkicA added the enhancement New feature or request label Feb 16, 2023
@MujkicA MujkicA self-assigned this Feb 16, 2023
digorithm
digorithm previously approved these changes Feb 20, 2023
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

LGTM -- just left a few clarification questions!

packages/fuels-programs/src/contract.rs Outdated Show resolved Hide resolved
packages/fuels/tests/contracts.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@segfault-magnet segfault-magnet left a comment

Choose a reason for hiding this comment

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

Reviewed alongside @hal3e. We've put our bigger impacting suggestions in a separate branch, you can compare the differences here.

Reasons for the suggestions:

  1. If a revert happened not because of a missing variable or message output then the code assumes that it must have been a missing contract input. This isn't true.
  2. The message inside expect_err in the abovementioned case will never be propagated. The result in question will always be an Err at that place in the code so expect_err will never panic. If the original intent was for the message to be propagated then our changes offer one way of achieving that.
  3. The current code assumes there can only be one revert reason (since only one match branch can be selected). Although this is probably true atm, it costs nothing to write it in a manner that can support multiple reverts.

We haven't discussed the MultiCallHandler's estimations. Perhaps they might benefit from the same suggestions.

packages/fuels-programs/src/contract.rs Outdated Show resolved Hide resolved
packages/fuels-programs/src/contract.rs Show resolved Hide resolved
Copy link
Contributor

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Salka1988 Salka1988 left a comment

Choose a reason for hiding this comment

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

👍

@MujkicA MujkicA merged commit 5a303f7 into master Feb 27, 2023
@MujkicA MujkicA deleted the mujkica/add_message_output_estimation branch February 27, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add output message tx dependency estimation
5 participants