API Review cleanup (#619)

* fix(service): rename WrappedService params for consistency

* docs(oer): add comment on `put_var_octet_string_length`

* fix: rename RouteManagerStore to CcpRoutingStore

Author: gakonst

crates/ilp-node/src/node.rs

@@ -28,7 +28,7 @@ use hex::FromHex;
 use interledger::{
     api::{NodeApi, NodeStore},
     btp::{btp_service_as_filter, connect_client, BtpOutgoingService, BtpStore},
-    ccp::{CcpRouteManagerBuilder, CcpRoutingAccount, RouteManagerStore, RoutingRelation},
+    ccp::{CcpRouteManagerBuilder, CcpRoutingAccount, CcpRoutingStore, RoutingRelation},
     errors::*,
     http::{HttpClientService, HttpServer as IlpOverHttpServer, HttpStore},
     ildcp::IldcpService,
@@ -284,7 +284,7 @@ impl InterledgerNode {
             + BalanceStore
             + SettlementStore<Account = Account>
             + RouterStore<Account = Account>
-            + RouteManagerStore<Account = Account>
+            + CcpRoutingStore<Account = Account>
             + RateLimitStore<Account = Account>
             + LeftoversStore<AccountId = Uuid, AssetType = BigUint>
             + IdempotentStore

crates/interledger-ccp/src/lib.rs

@@ -10,7 +10,7 @@
 //! we know about.
 
 use async_trait::async_trait;
-use interledger_errors::RouteManagerStoreError;
+use interledger_errors::CcpRoutingStoreError;
 use interledger_service::Account;
 use std::collections::HashMap;
 use std::{fmt, str::FromStr};
@@ -101,30 +101,30 @@ type LocalAndConfiguredRoutes<T> = (Routes<T>, Routes<T>);
 
 /// Store trait for managing the routes broadcast and set over Connector to Connector protocol
 #[async_trait]
-pub trait RouteManagerStore: Clone {
+pub trait CcpRoutingStore: Clone {
     type Account: CcpRoutingAccount;
 
     // TODO should we have a way to only get the details for specific routes?
     /// Gets the local and manually configured routes
     async fn get_local_and_configured_routes(
         &self,
-    ) -> Result<LocalAndConfiguredRoutes<Self::Account>, RouteManagerStoreError>;
+    ) -> Result<LocalAndConfiguredRoutes<Self::Account>, CcpRoutingStoreError>;
 
     /// Gets all accounts which the node should send routes to (Peer and Child accounts)
     /// The caller can also pass a vector of account ids to be ignored
     async fn get_accounts_to_send_routes_to(
         &self,
         ignore_accounts: Vec<Uuid>,
-    ) -> Result<Vec<Self::Account>, RouteManagerStoreError>;
+    ) -> Result<Vec<Self::Account>, CcpRoutingStoreError>;
 
     /// Gets all accounts which the node should receive routes to (Peer and Parent accounts)
     async fn get_accounts_to_receive_routes_from(
         &self,
-    ) -> Result<Vec<Self::Account>, RouteManagerStoreError>;
+    ) -> Result<Vec<Self::Account>, CcpRoutingStoreError>;
 
     /// Sets the new routes to the store (prefix -> account)
     async fn set_routes(
         &mut self,
         routes: impl IntoIterator<Item = (String, Self::Account)> + Send + 'async_trait,
-    ) -> Result<(), RouteManagerStoreError>;
+    ) -> Result<(), CcpRoutingStoreError>;
 }

crates/interledger-ccp/src/server.rs

@@ -4,11 +4,11 @@ use crate::{
         CCP_RESPONSE, CCP_UPDATE_DESTINATION,
     },
     routing_table::RoutingTable,
-    CcpRoutingAccount, RouteManagerStore, RoutingRelation,
+    CcpRoutingAccount, CcpRoutingStore, RoutingRelation,
 };
 use async_trait::async_trait;
 use futures::future::join_all;
-use interledger_errors::RouteManagerStoreError;
+use interledger_errors::CcpRoutingStoreError;
 use interledger_packet::{Address, ErrorCode, RejectBuilder};
 use interledger_service::{
     Account, AddressStore, IlpResult, IncomingRequest, IncomingService, OutgoingRequest,
@@ -54,6 +54,8 @@ fn hash(preimage: &[u8; 32]) -> [u8; 32] {
 
 type NewAndWithdrawnRoutes = (Vec<Route>, Vec<String>);
 
+/// Builder for [CcpRouteManager](./CcpRouteManager.html)
+/// See documentation on fields for more details.
 pub struct CcpRouteManagerBuilder<I, O, S> {
     /// The next request handler that will be used both to pass on requests that are not CCP messages.
     next_incoming: I,
@@ -72,7 +74,7 @@ impl<I, O, S, A> CcpRouteManagerBuilder<I, O, S>
 where
     I: IncomingService<A> + Clone + Send + Sync + 'static,
     O: OutgoingService<A> + Clone + Send + Sync + 'static,
-    S: AddressStore + RouteManagerStore<Account = A> + Clone + Send + Sync + 'static,
+    S: AddressStore + CcpRoutingStore<Account = A> + Clone + Send + Sync + 'static,
     A: CcpRoutingAccount + Send + Sync + 'static,
 {
     pub fn new(ilp_address: Address, store: S, outgoing: O, next_incoming: I) -> Self {
@@ -179,7 +181,7 @@ impl<I, O, S, A> CcpRouteManager<I, O, S, A>
 where
     I: IncomingService<A> + Clone + Send + Sync + 'static,
     O: OutgoingService<A> + Clone + Send + Sync + 'static,
-    S: AddressStore + RouteManagerStore<Account = A> + Clone + Send + Sync + 'static,
+    S: AddressStore + CcpRoutingStore<Account = A> + Clone + Send + Sync + 'static,
     A: CcpRoutingAccount + Send + Sync + 'static,
 {
     /// Returns a future that will trigger this service to update its routes and broadcast
@@ -210,7 +212,7 @@ where
         }
     }
 
-    pub async fn broadcast_routes(&self) -> Result<(), RouteManagerStoreError> {
+    pub async fn broadcast_routes(&self) -> Result<(), CcpRoutingStoreError> {
         self.update_best_routes(None).await?;
         self.send_route_updates().await
     }
@@ -518,7 +520,7 @@ where
     async fn update_best_routes(
         &self,
         prefixes: Option<Vec<String>>,
-    ) -> Result<(), RouteManagerStoreError> {
+    ) -> Result<(), CcpRoutingStoreError> {
         let local_table = self.local_table.clone();
         let forwarding_table = self.forwarding_table.clone();
         let forwarding_table_updates = self.forwarding_table_updates.clone();
@@ -657,7 +659,7 @@ where
     }
 
     /// Send RouteUpdateRequests to all peers that we send routing messages to
-    async fn send_route_updates(&self) -> Result<(), RouteManagerStoreError> {
+    async fn send_route_updates(&self) -> Result<(), CcpRoutingStoreError> {
         let self_clone = self.clone();
         let unavailable_accounts = self.unavailable_accounts.clone();
         // Check which accounts we should skip this iteration
@@ -969,7 +971,7 @@ impl<I, O, S, A> IncomingService<A> for CcpRouteManager<I, O, S, A>
 where
     I: IncomingService<A> + Clone + Send + Sync + 'static,
     O: OutgoingService<A> + Clone + Send + Sync + 'static,
-    S: AddressStore + RouteManagerStore<Account = A> + Clone + Send + Sync + 'static,
+    S: AddressStore + CcpRoutingStore<Account = A> + Clone + Send + Sync + 'static,
     A: CcpRoutingAccount + Send + Sync + 'static,
 {
     /// Handle the IncomingRequest if it is a CCP protocol message or

crates/interledger-ccp/src/test_helpers.rs

@@ -2,7 +2,7 @@
 use super::*;
 use crate::{packet::CCP_RESPONSE, server::CcpRouteManager};
 use async_trait::async_trait;
-use interledger_errors::{AddressStoreError, RouteManagerStoreError};
+use interledger_errors::{AddressStoreError, CcpRoutingStoreError};
 use interledger_packet::{Address, ErrorCode, RejectBuilder};
 use interledger_service::{
     incoming_service_fn, outgoing_service_fn, AddressStore, IncomingService, OutgoingRequest,
@@ -126,20 +126,19 @@ impl AddressStore for TestStore {
 }
 
 #[async_trait]
-impl RouteManagerStore for TestStore {
+impl CcpRoutingStore for TestStore {
     type Account = TestAccount;
 
     async fn get_local_and_configured_routes(
         &self,
-    ) -> Result<(RoutingTable<TestAccount>, RoutingTable<TestAccount>), RouteManagerStoreError>
-    {
+    ) -> Result<(RoutingTable<TestAccount>, RoutingTable<TestAccount>), CcpRoutingStoreError> {
         Ok((self.local.clone(), self.configured.clone()))
     }
 
     async fn get_accounts_to_send_routes_to(
         &self,
         ignore_accounts: Vec<Uuid>,
-    ) -> Result<Vec<TestAccount>, RouteManagerStoreError> {
+    ) -> Result<Vec<TestAccount>, CcpRoutingStoreError> {
         let mut accounts: Vec<TestAccount> = self
             .local
             .values()
@@ -156,7 +155,7 @@ impl RouteManagerStore for TestStore {
 
     async fn get_accounts_to_receive_routes_from(
         &self,
-    ) -> Result<Vec<TestAccount>, RouteManagerStoreError> {
+    ) -> Result<Vec<TestAccount>, CcpRoutingStoreError> {
         let mut accounts: Vec<TestAccount> = self
             .local
             .values()
@@ -172,7 +171,7 @@ impl RouteManagerStore for TestStore {
     async fn set_routes(
         &mut self,
         routes: impl IntoIterator<Item = (String, TestAccount)> + Send + 'async_trait,
-    ) -> Result<(), RouteManagerStoreError> {
+    ) -> Result<(), CcpRoutingStoreError> {
         *self.routes.lock() = HashMap::from_iter(routes.into_iter());
         Ok(())
     }

crates/interledger-errors/src/ccprouting_store_error.rs

@@ -0,0 +1,49 @@
+use super::{AccountStoreError, NodeStoreError};
+use crate::error::ApiError;
+use std::error::Error as StdError;
+use thiserror::Error;
+
+/// Errors for the CcpRoutingStore
+#[derive(Error, Debug)]
+#[non_exhaustive]
+pub enum CcpRoutingStoreError {
+    #[error("{0}")]
+    Other(#[from] Box<dyn StdError + Send + 'static>),
+}
+
+impl From<AccountStoreError> for CcpRoutingStoreError {
+    fn from(src: AccountStoreError) -> Self {
+        CcpRoutingStoreError::Other(Box::new(src))
+    }
+}
+
+impl From<NodeStoreError> for CcpRoutingStoreError {
+    fn from(src: NodeStoreError) -> Self {
+        CcpRoutingStoreError::Other(Box::new(src))
+    }
+}
+
+impl From<CcpRoutingStoreError> for ApiError {
+    fn from(src: CcpRoutingStoreError) -> Self {
+        match src {
+            _ => ApiError::method_not_allowed(),
+        }
+    }
+}
+
+#[cfg(feature = "warp_errors")]
+impl From<CcpRoutingStoreError> for warp::Rejection {
+    fn from(src: CcpRoutingStoreError) -> Self {
+        ApiError::from(src).into()
+    }
+}
+
+#[cfg(feature = "redis_errors")]
+use redis::RedisError;
+
+#[cfg(feature = "redis_errors")]
+impl From<RedisError> for CcpRoutingStoreError {
+    fn from(src: RedisError) -> CcpRoutingStoreError {
+        CcpRoutingStoreError::Other(Box::new(src))
+    }
+}

crates/interledger-errors/src/lib.rs

@@ -14,8 +14,8 @@ pub use http_store_error::HttpStoreError;
 mod btp_store_error;
 pub use btp_store_error::BtpStoreError;
 
-mod routemanager_store_error;
-pub use routemanager_store_error::RouteManagerStoreError;
+mod ccprouting_store_error;
+pub use ccprouting_store_error::CcpRoutingStoreError;
 
 mod balance_store_error;
 pub use balance_store_error::BalanceStoreError;

crates/interledger-errors/src/routemanager_store_error.rs

@@ -144,7 +144,7 @@ pub trait MutBufOerExt: BufMut + Sized {
         self.put(buf);
     }
 
-    #[doc(hidden)]
+    /// Encodes the length of a datatype as variable-length octet encoded unsigned integer and puts it into `Buf`
     #[inline]
     fn put_var_octet_string_length(&mut self, length: usize) {
         if length < 128 {

crates/interledger-packet/src/oer.rs

@@ -258,18 +258,18 @@ pub struct WrappedService<F, I, A> {
     account_type: PhantomData<A>,
 }
 
-impl<F, IO, A, R> WrappedService<F, IO, A>
+impl<F, I, A, R> WrappedService<F, I, A>
 where
     F: Send + Sync + Fn(IncomingRequest<A>, Box<dyn IncomingService<A> + Send>) -> R,
     R: Future<Output = IlpResult>,
-    IO: IncomingService<A> + Clone,
+    I: IncomingService<A> + Clone,
     A: Account,
 {
     /// Wrap the given service such that the provided function will
     /// be called to handle each request. That function can
     /// return immediately, modify the request before passing it on,
     /// and/or handle the result of calling the inner service.
-    pub fn wrap_incoming(inner: IO, f: F) -> Self {
+    pub fn wrap_incoming(inner: I, f: F) -> Self {
         WrappedService {
             f,
             inner: Arc::new(inner),
@@ -279,30 +279,30 @@ where
 }
 
 #[async_trait]
-impl<F, IO, A, R> IncomingService<A> for WrappedService<F, IO, A>
+impl<F, I, A, R> IncomingService<A> for WrappedService<F, I, A>
 where
     F: Send + Sync + Fn(IncomingRequest<A>, Box<dyn IncomingService<A> + Send>) -> R,
     R: Future<Output = IlpResult> + Send + 'static,
-    IO: IncomingService<A> + Send + Sync + Clone + 'static,
+    I: IncomingService<A> + Send + Sync + Clone + 'static,
     A: Account + Sync,
 {
     async fn handle_request(&mut self, request: IncomingRequest<A>) -> IlpResult {
         (self.f)(request, Box::new((*self.inner).clone())).await
     }
 }
 
-impl<F, IO, A, R> WrappedService<F, IO, A>
+impl<F, O, A, R> WrappedService<F, O, A>
 where
     F: Send + Sync + Fn(OutgoingRequest<A>, Box<dyn OutgoingService<A> + Send>) -> R,
     R: Future<Output = IlpResult>,
-    IO: OutgoingService<A> + Clone,
+    O: OutgoingService<A> + Clone,
     A: Account,
 {
     /// Wrap the given service such that the provided function will
     /// be called to handle each request. That function can
     /// return immediately, modify the request before passing it on,
     /// and/or handle the result of calling the inner service.
-    pub fn wrap_outgoing(inner: IO, f: F) -> Self {
+    pub fn wrap_outgoing(inner: O, f: F) -> Self {
         WrappedService {
             f,
             inner: Arc::new(inner),
@@ -312,11 +312,11 @@ where
 }
 
 #[async_trait]
-impl<F, IO, A, R> OutgoingService<A> for WrappedService<F, IO, A>
+impl<F, O, A, R> OutgoingService<A> for WrappedService<F, O, A>
 where
     F: Send + Sync + Fn(OutgoingRequest<A>, Box<dyn OutgoingService<A> + Send>) -> R,
     R: Future<Output = IlpResult> + Send + 'static,
-    IO: OutgoingService<A> + Send + Sync + Clone + 'static,
+    O: OutgoingService<A> + Send + Sync + Clone + 'static,
     A: Account,
 {
     async fn send_request(&mut self, request: OutgoingRequest<A>) -> IlpResult {