Refactor CLI app to handle BreezServices lifetime
#1213
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Stop using the static variable since they are not dropped: https://doc.rust-lang.org/reference/items/static-items.html#r-items.static.lifetime * Add an `abort` command to mimic crash behaviors (useful for testing)
roeierez
requested changes
May 12, 2025
|
|
||
| async fn connect(&mut self, req: ConnectRequest) -> Result<()> { | ||
| let service = BreezServices::connect(req, Box::new(CliEventListener {})).await?; | ||
| ensure!(self.sdk.is_none(), "Breez Services already initialized"); |
There was a problem hiding this comment.
Perhaps better to validate before we connect?
| Ok(format!("Closing transaction ids:\n{tx_ids:?}")) | ||
| } | ||
| Commands::Disconnect {} => { | ||
| self.sdk()?.disconnect().await?; |
There was a problem hiding this comment.
Now that it is not static anymore I think it would be nice to nullify the sdk instance allowing the user to re-connect again.
andrei-21
force-pushed
the
feature/drop-sdk
branch
from
12c45c6 to
3dff48b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the CLI app to remove the static BreezServices, replacing it with a properly scoped CommandHandler that manages the services’ lifecycle, and adds an abort command for testing crash behaviors.
- Refactored the command handling by creating a CommandHandler struct and updating its associated methods.
- Removed the static variable for BreezServices and added a new connect method to manage its lifecycle.
- Introduced a new Abort command to simulate a crash for testing purposes.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/sdk-cli/src/main.rs | Updated to instantiate and use CommandHandler instead of a static function. |
| tools/sdk-cli/src/commands.rs | Added a new Abort command for immediate termination. |
| tools/sdk-cli/src/command_handlers.rs | Refactored BreezServices usage by replacing static initialization with a CommandHandler struct and updated command handling logic. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
abortcommand to mimic crash behaviors (useful for testing)