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

Contract input set estimation #628

Merged
merged 34 commits into from
Nov 15, 2022
Merged

Conversation

Salka1988
Copy link
Member

@Salka1988 Salka1988 commented Oct 15, 2022

Add set_contracts_automatic which automatically adds missing contract_ids.

blocked by:
FuelLabs/fuel-core#702

In addition, we will need a new release of fuel-core that supports transparent_receipt::Receipt' as the return value of the dry_run' function.

Important:
I noticed that introducing the contract_id in Panic and all the necessary logic in fuel-core drastically increases the gas_fee, which can be seen from the tests.

@Salka1988 Salka1988 self-assigned this Oct 15, 2022
@Salka1988 Salka1988 marked this pull request as draft October 15, 2022 22:38
@Salka1988 Salka1988 linked an issue Oct 15, 2022 that may be closed by this pull request
# Conflicts:
#	packages/fuels-contract/Cargo.toml
#	packages/fuels-contract/src/contract.rs
#	packages/fuels-core/Cargo.toml
#	packages/fuels-signers/Cargo.toml
#	packages/fuels-signers/src/provider.rs
#	packages/fuels-signers/src/wallet.rs
#	packages/fuels-test-helpers/Cargo.toml
#	packages/fuels-test-helpers/src/lib.rs
#	packages/fuels-types/Cargo.toml
#	packages/fuels/Cargo.toml
#	packages/fuels/tests/harness.rs
@Salka1988 Salka1988 requested a review from a team October 19, 2022 10:07
@Salka1988 Salka1988 marked this pull request as ready for review October 19, 2022 15:17
@Salka1988 Salka1988 force-pushed the Salka1988/contract_set_estimation branch from 4a56309 to 13ae5b5 Compare October 24, 2022 13:44
# Conflicts:
#	examples/contracts/src/lib.rs
#	examples/predicates/src/lib.rs
#	packages/fuels-contract/Cargo.toml
#	packages/fuels-contract/src/contract.rs
#	packages/fuels-contract/src/script.rs
#	packages/fuels-core/Cargo.toml
#	packages/fuels-signers/Cargo.toml
#	packages/fuels-signers/src/provider.rs
#	packages/fuels-signers/src/wallet.rs
#	packages/fuels-test-helpers/Cargo.toml
#	packages/fuels-types/Cargo.toml
#	packages/fuels-types/src/errors.rs
#	packages/fuels/Cargo.toml
#	packages/fuels/tests/predicates.rs
#	packages/fuels/tests/providers.rs
@Salka1988
Copy link
Member Author

Salka1988 commented Nov 2, 2022

Blocked by FuelLabs/fuel-vm#261
Now we only need new release of fuel-core and it should work @Voxelot

…bs/fuels-rs into Salka1988/contract_set_estimation

� Conflicts:
�	packages/fuels-contract/Cargo.toml
�	packages/fuels-signers/Cargo.toml
�	packages/fuels-test-helpers/Cargo.toml
�	packages/fuels/Cargo.toml
@Salka1988 Salka1988 removed the blocked label Nov 5, 2022
@Salka1988 Salka1988 force-pushed the Salka1988/contract_set_estimation branch from ce04278 to 8cb94fe Compare November 6, 2022 00:12
@Salka1988 Salka1988 removed the blocked label Nov 10, 2022
@digorithm digorithm added the enhancement New feature or request label Nov 10, 2022
Copy link
Contributor

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

LGTM. Good work 🎉

packages/fuels/tests/contracts.rs Show resolved Hide resolved
packages/fuels-contract/src/contract.rs Outdated Show resolved Hide resolved
packages/fuels-contract/src/contract.rs Outdated Show resolved Hide resolved
packages/fuels-contract/src/contract.rs Outdated Show resolved Hide resolved
packages/fuels-contract/src/script.rs Outdated Show resolved Hide resolved
packages/fuels/tests/contracts.rs Show resolved Hide resolved
@Salka1988 Salka1988 requested review from MujkicA and a team November 10, 2022 22:05
MujkicA
MujkicA previously approved these changes Nov 11, 2022
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.

Very good work! Left one suggestion to improve error handling.

let contract_id = Bech32ContractId::from(*receipt.contract_id().unwrap());
self = self.append_contract(contract_id);
} else {
return Err(result.err().unwrap());
Copy link
Member

Choose a reason for hiding this comment

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

This error here will be very cryptic when returned to the user.

The user will be trying to estimate_tx_dependencies, but if it is an Error::RevertTransactionError and for some weird reason that's out of our control, ContractCall::find_contract_not_in_inputs returns None, the Err here will be very hard to understand.

So we should return this Err with a string saying something like "Couldn't estimate tx dependencies because we couldn't find the missing contract input".

Copy link
Contributor

@MujkicA MujkicA Nov 14, 2022

Choose a reason for hiding this comment

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

If ContractCall::find_contract_not_in_inputs returns None it should mean that the Error::RevertTransactionError was due to reasons other than missing output variables or contract inputs. We should forward the original error to the user in that case since it cannot be fixed by estimating dependencies anyways. ~~
I need to improve my reading skills

Copy link
Member Author

Choose a reason for hiding this comment

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

This was the initial idea.

packages/fuels-contract/src/contract.rs Outdated Show resolved Hide resolved
packages/fuels-contract/src/contract.rs Show resolved Hide resolved
@Salka1988 Salka1988 enabled auto-merge (squash) November 11, 2022 23:48
@Salka1988 Salka1988 disabled auto-merge November 11, 2022 23:51
@Salka1988 Salka1988 requested a review from digorithm November 12, 2022 16:23
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 -- great work @Salka1988!

@Salka1988 Salka1988 merged commit c05056f into master Nov 15, 2022
@Salka1988 Salka1988 deleted the Salka1988/contract_set_estimation branch November 15, 2022 16:52
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.

Contract input set estimation
4 participants