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

feat!: make functions return Result and implement Node Killer #1067

Merged
merged 94 commits into from
Oct 14, 2023

Conversation

Salka1988
Copy link
Member

@Salka1988 Salka1988 commented Jul 28, 2023

Refactored functions to return Result for better error handling.

  • launch_provider_and_get_wallet()
  • launch_custom_provider_and_get_wallets()
  • setup_test_provider()
  • setup_test_client()

I've implemented FuelService and added support for the fuel_core_services::Service trait. This allows us to have finer control over the behavior of FuelNode;

pub struct FuelService {
    #[cfg(feature = "fuel-core-lib")]
    service: CoreFuelService,
    #[cfg(not(feature = "fuel-core-lib"))]
    service: BinFuelService,
    bound_address: SocketAddr,
}

Checklist

  • 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.

@Salka1988 Salka1988 added dev-experience breaking Introduces or requires breaking changes labels Jul 28, 2023
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.

Thanks for the nice refactor and devX improvement!

packages/fuels-test-helpers/src/accounts.rs Outdated Show resolved Hide resolved
packages/fuels-test-helpers/src/accounts.rs Outdated Show resolved Hide resolved
packages/fuels-test-helpers/src/lib.rs Outdated Show resolved Hide resolved
packages/fuels-test-helpers/src/lib.rs Outdated Show resolved Hide resolved
packages/fuels/tests/bindings.rs Outdated Show resolved Hide resolved
@Salka1988 Salka1988 requested a review from hal3e August 2, 2023 11:51
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.

This is going to break every test out there. It should definitely be fixed while we have the time, my thinking is just whether we want to make some more changes to this so that we may at least have something more to show for it besides better panic handling.

For example, one thing we've been putting off for so long is the ability to kill the spawned node. Useful when users want to test the behavior of their app when the node is down.

Currently, they need to kill the tokio runtime since a handle was hidden somewhere inside that keeps the node alive.

Ideally, we'd ship this out with that feature (and maybe any other pressing ones) and at least make a feature-rich-breaking change.

@FuelLabs/sdk-rust

@Salka1988
Copy link
Member Author

This is going to break every test out there. It should definitely be fixed while we have the time, my thinking is just whether we want to make some more changes to this so that we may at least have something more to show for it besides better panic handling.

For example, one thing we've been putting off for so long is the ability to kill the spawned node. Useful when users want to test the behavior of their app when the node is down.

Currently, they need to kill the tokio runtime since a handle was hidden somewhere inside that keeps the node alive.

Ideally, we'd ship this out with that feature (and maybe any other pressing ones) and at least make a feature-rich-breaking change.

@FuelLabs/sdk-rust

I could do it in this PR, but I think it is better to make a new PR. I will do as @FuelLabs/sdk-rust decides. Anyway, I'm going to start implementing the spawned node killer.

MujkicA
MujkicA previously approved these changes Oct 13, 2023
Br1ght0ne
Br1ght0ne previously approved these changes Oct 13, 2023
iqdecay
iqdecay previously approved these changes Oct 13, 2023
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.

I like the way this looks now! I have some change requests though...

packages/fuels-test-helpers/src/lib.rs Outdated Show resolved Hide resolved
packages/fuels-test-helpers/src/fuel_bin_service.rs Outdated Show resolved Hide resolved
packages/fuels-test-helpers/src/fuel_bin_service.rs Outdated Show resolved Hide resolved
packages/fuels/Cargo.toml Show resolved Hide resolved
packages/fuels-test-helpers/src/fuel_bin_service.rs Outdated Show resolved Hide resolved
packages/fuels-test-helpers/src/lib.rs Outdated Show resolved Hide resolved
packages/fuels-test-helpers/src/service.rs Outdated Show resolved Hide resolved
packages/fuels-test-helpers/src/fuel_bin_service.rs Outdated Show resolved Hide resolved
packages/fuels-test-helpers/src/fuel_bin_service.rs Outdated Show resolved Hide resolved
packages/fuels-test-helpers/src/fuel_bin_service.rs Outdated Show resolved Hide resolved
Co-authored-by: hal3e <git@hal3e.io>
@Salka1988 Salka1988 dismissed stale reviews from iqdecay, Br1ght0ne, and MujkicA via b912fd4 October 13, 2023 14:06
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.

Great work!

@Salka1988 Salka1988 merged commit d416a49 into master Oct 14, 2023
39 checks passed
@Salka1988 Salka1988 deleted the Salka1988/return_result branch October 14, 2023 10:12
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 dev-experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants