Fix: Remove Unnecessary &mut self Requirements from LightningNode Trait #284
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
carlaKC
reviewed
Jun 13, 2025
simln-lib/src/sim_node.rs
Outdated
| @@ -495,7 +495,7 @@ pub struct SimNode<'a, T: SimNetwork> { | |||
| /// The underlying execution network that will be responsible for dispatching payments. | |||
| network: Arc<Mutex<T>>, | |||
| /// Tracks the channel that will provide updates for payments by hash. | |||
| in_flight: HashMap<PaymentHash, Receiver<Result<PaymentResult, LightningError>>>, | |||
| in_flight: Mutex<HashMap<PaymentHash, Receiver<Result<PaymentResult, LightningError>>>>, // HashMap<PaymentHash, Receiver<Result<PaymentResult, LightningError>>>, | |||
There was a problem hiding this comment.
nit: unnecessary comment at the end?
simln-lib/src/sim_node.rs
Outdated
| @@ -523,7 +523,7 @@ impl<'a, T: SimNetwork> SimNode<'a, T> { | |||
| SimNode { | |||
| info, | |||
| network: payment_network, | |||
| in_flight: HashMap::new(), | |||
| in_flight: HashMap::new().into(), | |||
There was a problem hiding this comment.
nitnit: this reads a bit odd in my very subjective opinion, just use Mutex::new
simln-lib/src/sim_node.rs
Outdated
Comment on lines
633
to
634
| let mut in_flight = self.in_flight.lock().await; | ||
| match in_flight.entry(payment_hash) { |
There was a problem hiding this comment.
IMO can inline things like this without losing much readability
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.
This PR refactors the LightningNode trait and its implementations to remove unnecessary &mut self references for methods that do not mutate internal state.
Motivation
Previously, several trait methods like get_network and list_channels required &mut self, even though they only retrieved information. This requirement leaked internal implementation details—specifically, that the underlying client field required a mutable reference to make gRPC or HTTP calls.
This design was misleading and unnecessarily restricted usage of the trait, e.g. preventing multiple read-only operations from being performed concurrently on the same node.
Changes
Wrapped client fields (e.g. NodeClient, EclairClient, etc.) inside tokio::sync::Mutex.
Updated all client usages to acquire a mutable lock via let mut client = self.client.lock().await;.
Updated trait method signatures to require &self instead of &mut self where applicable.
Ensured thread-safe, concurrent-friendly behavior without sacrificing correctness or clarity.