Fix clippy issues, enforce in CI

We weren't checking for any clippy issues. This fixes most of them and adds a few exceptions for ones not worth fixing or ones that will be fixed with future changes.

Author: benthecarman

.github/workflows/build.yml

@@ -28,6 +28,7 @@ jobs:
         run: |
           curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile=minimal --default-toolchain ${{ matrix.toolchain }}
           rustup override set ${{ matrix.toolchain }}
+          if [ "${{ matrix.msrv }}" = "true" ]; then rustup component add clippy; fi
       - name: Check formatting
         if: matrix.check-fmt
         run: rustup component add rustfmt && cargo fmt --all -- --check
@@ -37,6 +38,9 @@ jobs:
           echo "No packages need pinning for MSRV ${{ matrix.toolchain }}"
       - name: Build on Rust ${{ matrix.toolchain }}
         run: cargo build --verbose --color always
+      - name: Check clippy if on msrv
+        if: matrix.msrv
+        run: cargo clippy --all-features -- -D warnings
       - name: Test on Rust ${{ matrix.toolchain }}
         run: cargo test
       - name: Cargo check release on Rust ${{ matrix.toolchain }}

ldk-server-protos/Cargo.toml

@@ -5,6 +5,13 @@ edition = "2021"
 
 build = "build.rs"
 
+# We use a cfg instead of a feature for genproto to prevent it from being
+# enabled with --all-features. Proto generation is a developer-only tool that
+# requires external dependencies (protoc) and shouldn't be triggered accidentally.
+# This lint configuration tells Cargo that genproto is an expected custom cfg.
+[lints.rust]
+unexpected_cfgs = { level = "warn", check-cfg = ['cfg(genproto)'] }
+
 [features]
 default = []
 serde = ["dep:serde", "dep:bytes"]

ldk-server/src/api/bolt11_send.rs

@@ -9,7 +9,7 @@ use std::str::FromStr;
 pub(crate) fn handle_bolt11_send_request(
 	context: Context, request: Bolt11SendRequest,
 ) -> Result<Bolt11SendResponse, LdkServerError> {
-	let invoice = Bolt11Invoice::from_str(&request.invoice.as_str())
+	let invoice = Bolt11Invoice::from_str(request.invoice.as_str())
 		.map_err(|_| ldk_node::NodeError::InvalidInvoice)?;
 
 	let route_parameters = match request.route_parameters {

ldk-server/src/api/bolt12_send.rs

@@ -10,7 +10,7 @@ pub(crate) fn handle_bolt12_send_request(
 	context: Context, request: Bolt12SendRequest,
 ) -> Result<Bolt12SendResponse, LdkServerError> {
 	let offer =
-		Offer::from_str(&request.offer.as_str()).map_err(|_| ldk_node::NodeError::InvalidOffer)?;
+		Offer::from_str(request.offer.as_str()).map_err(|_| ldk_node::NodeError::InvalidOffer)?;
 
 	let route_parameters = match request.route_parameters {
 		Some(params) => {

ldk-server/src/api/error.rs

@@ -29,13 +29,11 @@ impl fmt::Display for LdkServerError {
 }
 
 #[derive(Clone, Debug, PartialEq, Eq)]
+#[allow(clippy::enum_variant_names)]
 pub(crate) enum LdkServerErrorCode {
 	/// Please refer to [`protos::error::ErrorCode::InvalidRequestError`].
 	InvalidRequestError,
 
-	/// Please refer to [`protos::error::ErrorCode::AuthError`].
-	AuthError,
-
 	/// Please refer to [`protos::error::ErrorCode::LightningError`].
 	LightningError,
 
@@ -47,7 +45,6 @@ impl fmt::Display for LdkServerErrorCode {
 	fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
 		match self {
 			LdkServerErrorCode::InvalidRequestError => write!(f, "InvalidRequestError"),
-			LdkServerErrorCode::AuthError => write!(f, "AuthError"),
 			LdkServerErrorCode::LightningError => write!(f, "LightningError"),
 			LdkServerErrorCode::InternalServerError => write!(f, "InternalServerError"),
 		}

ldk-server/src/api/get_payment_details.rs

@@ -19,9 +19,7 @@ pub(crate) fn handle_get_payment_details_request(
 
 	let payment_details = context.node.payment(&PaymentId(payment_id_bytes));
 
-	let response = GetPaymentDetailsResponse {
-		payment: payment_details.map(|payment| payment_to_proto(payment)),
-	};
+	let response = GetPaymentDetailsResponse { payment: payment_details.map(payment_to_proto) };
 
 	Ok(response)
 }

ldk-server/src/api/list_channels.rs

@@ -6,7 +6,7 @@ use ldk_server_protos::api::{ListChannelsRequest, ListChannelsResponse};
 pub(crate) fn handle_list_channels_request(
 	context: Context, _request: ListChannelsRequest,
 ) -> Result<ListChannelsResponse, LdkServerError> {
-	let channels = context.node.list_channels().into_iter().map(|c| channel_to_proto(c)).collect();
+	let channels = context.node.list_channels().into_iter().map(channel_to_proto).collect();
 
 	let response = ListChannelsResponse { channels };
 	Ok(response)

ldk-server/src/api/onchain_send.rs

@@ -18,7 +18,7 @@ pub(crate) fn handle_onchain_send_request(
 			)
 		})?;
 
-	let fee_rate = request.fee_rate_sat_per_vb.map(FeeRate::from_sat_per_vb).flatten();
+	let fee_rate = request.fee_rate_sat_per_vb.and_then(FeeRate::from_sat_per_vb);
 	let txid = match (request.amount_sats, request.send_all) {
 		(Some(amount_sats), None) => {
 			context.node.onchain_payment().send_to_address(&address, amount_sats, fee_rate)?

ldk-server/src/io/events/event_publisher.rs

@@ -41,9 +41,12 @@ pub trait EventPublisher: Send + Sync {
 	async fn publish(&self, event: EventEnvelope) -> Result<(), LdkServerError>;
 }
 
+/// A no-op implementation of the [`EventPublisher`] trait.
+#[cfg(not(feature = "events-rabbitmq"))]
 pub(crate) struct NoopEventPublisher;
 
 #[async_trait]
+#[cfg(not(feature = "events-rabbitmq"))]
 impl EventPublisher for NoopEventPublisher {
 	/// Publishes an event to a no-op sink, effectively discarding it.
 	///

ldk-server/src/io/persist/paginated_kv_store.rs

@@ -49,25 +49,6 @@ pub trait PaginatedKVStore: Send + Sync {
 		&self, primary_namespace: &str, secondary_namespace: &str, key: &str, time: i64, buf: &[u8],
 	) -> Result<(), io::Error>;
 
-	/// Removes any data that had previously been persisted under the given `key`.
-	///
-	/// If the `lazy` flag is set to `true`, the backend implementation might choose to lazily
-	/// remove the given `key` at some point in time after the method returns, e.g., as part of an
-	/// eventual batch deletion of multiple keys. As a consequence, subsequent calls to
-	/// [`PaginatedKVStore::list`] might include the removed key until the changes are actually persisted.
-	///
-	/// Note that while setting the `lazy` flag reduces the I/O burden of multiple subsequent
-	/// `remove` calls, it also influences the atomicity guarantees as lazy `remove`s could
-	/// potentially get lost on crash after the method returns. Therefore, this flag should only be
-	/// set for `remove` operations that can be safely replayed at a later time.
-	///
-	/// Returns successfully if no data will be stored for the given `primary_namespace`,
-	/// `secondary_namespace`, and `key`, independently of whether it was present before its
-	/// invocation or not.
-	fn remove(
-		&self, primary_namespace: &str, secondary_namespace: &str, key: &str, lazy: bool,
-	) -> Result<(), io::Error>;
-
 	/// Returns a paginated list of keys that are stored under the given `secondary_namespace` in
 	/// `primary_namespace`, ordered in descending order of `time`.
 	///