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

refactor!: remove NetworkInfo #1231

Merged
merged 6 commits into from
Dec 14, 2023
Merged

refactor!: remove NetworkInfo #1231

merged 6 commits into from
Dec 14, 2023

Conversation

hal3e
Copy link
Contributor

@hal3e hal3e commented Dec 11, 2023

When building a transaction with the SDK we currently ask for both NetworkInfo and a provider when building.
NetworkInfo is redundant as we can fetch all necessary information from the provider while building.

BREAKING CHANGE:

  • removed NetworkInfo and all related functions/methods
  • renamed DryRunner trait and added trait methods
  • removed ScritpTransactionBuilder::new and CreateTransactionBuilder::new
  • removed Provider::new

Checklist

  • I have updated the
  • I have added necessary labels.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@hal3e hal3e added tech-debt Improves code quality or safety breaking Introduces or requires breaking changes labels Dec 11, 2023
@hal3e hal3e self-assigned this Dec 11, 2023
digorithm
digorithm previously approved these changes Dec 12, 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.

Great work, @hal3e. This is an awesome UX simplification!

packages/fuels-accounts/src/provider/supported_versions.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.

I'd rather keep the old name even if we sneak one more method under.

This one is too generic. Might be a sign that we're looking at two traits: DryRunner, ChainInfoProvider.

@hal3e hal3e dismissed stale reviews from segfault-magnet and digorithm via 1d69011 December 12, 2023 16:17
@digorithm
Copy link
Member

I'd rather keep the old name even if we sneak one more method under.

This one is too generic. Might be a sign that we're looking at two traits: DryRunner, ChainInfoProvider.

Fair enough

@MujkicA
Copy link
Contributor

MujkicA commented Dec 13, 2023

The main point of the trait is to provide the used amount of gas.
Whether that is being done with a dry run or through some other means doesn't need to be exposed IMO.
I would vote for calling it something like GasEstimator

@hal3e
Copy link
Contributor Author

hal3e commented Dec 13, 2023

I have decided to leave the name DryRunner for now.

@hal3e hal3e merged commit c69cb81 into master Dec 14, 2023
39 checks passed
@hal3e hal3e deleted the hal3e/refactor-build branch December 14, 2023 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces or requires breaking changes tech-debt Improves code quality or safety
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants