Merge #1723

1723: Refactor deserialization and serialization of ledger and asset values r=mergify[bot] a=thomaseizinger

We make heavy use of serde's "from" and "into" features to explicitly model the format we expect on the HTTP API and the format we send out.

This allows us to shift most of the error handling to serde, which in turn gives us better error messages. They are not perfect but about as good as it gets if we stick to deserializing JSON with serde.

We could potentially have better error messages by:

- either taking apart the JSON manually but that would be a huge
coding effort and cumbersome to maintain because we could not make
use of many features of serde.
- simplifying the JSON model to reduce the complexity in the
deserialization process.

Fixes #1364.

As a side-benefit, we also remove a circular dependency between the `Ledger` & `Asset` traits and the `http_api` module. This brings us one step closer to the extracting a library.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Lucas Soriano del Pino <l.soriano.del.pino@gmail.com>

Author: 3 people

Cargo.lock

@@ -54,9 +54,8 @@ import { sleep } from "../lib/util";
                     },
                 });
 
-            expect(res.error).to.be.false;
-            expect(res.status).to.equal(201);
-            expect(res.header.location).to.be.a("string");
+            expect(res).to.have.status(201);
+            expect(res).to.have.header("location");
         });
 
         it("[Alice] Should not see any peers because the address did not resolve to the given PeerID", async () => {

api_tests/dry/peers_using_ip.ts

@@ -33,7 +33,7 @@ setTimeout(async function() {
             expect(body.entities).to.have.lengthOf(0);
         });
 
-        it("[Alice] Returns 400 Swap not supported for an unsupported combination of parameters", async () => {
+        it("[Alice] Returns 400 invalid body for an unsupported combination of parameters", async () => {
             const res = await request(alice.cndHttpApiUrl())
                 .post("/swaps/rfc003")
                 .send({
@@ -63,10 +63,10 @@ setTimeout(async function() {
                 "content-type",
                 "application/problem+json"
             );
-            expect(res.body.title).to.equal("Swap not supported.");
+            expect(res.body.title).to.equal("Invalid body.");
         });
 
-        it("[Alice] Returns 400 bad request for malformed requests", async () => {
+        it("[Alice] Returns 400 invalid body for malformed requests", async () => {
             const res = await request(alice.cndHttpApiUrl())
                 .post("/swaps/rfc003")
                 .send({
@@ -78,7 +78,7 @@ setTimeout(async function() {
                 "content-type",
                 "application/problem+json"
             );
-            expect(res.body.title).to.equal("Bad Request");
+            expect(res.body.title).to.equal("Invalid body.");
         });
 
         it("[Alice] Should have no peers before making a swap request", async () => {

api_tests/dry/sanity.ts

@@ -34,6 +34,7 @@ http-api-problem = "0.13"
 hyper = "0.12"
 lazy_static = "1"
 libp2p = { version = "0.13" }
+libp2p-core = { version = "0.13" }
 libp2p-comit = { path = "../libp2p-comit" }
 libsqlite3-sys = { version = ">=0.8.0, <0.13.0", features = ["bundled"] }
 log = { version = "0.4", features = ["serde"] }

cnd/Cargo.toml

@@ -15,7 +15,7 @@ use anyhow::Context;
 use blockchain_contracts::bitcoin::witness;
 use http_api_problem::HttpApiProblem;
 use serde::{Deserialize, Serialize};
-use std::convert::Infallible;
+use std::convert::{Infallible, TryInto};
 use warp::http::StatusCode;
 
 pub trait ToSirenAction {
@@ -246,7 +246,7 @@ impl IntoResponsePayload for ethereum::DeployContract {
                 amount,
                 gas_limit,
                 chain_id,
-                network: chain_id.into(),
+                network: chain_id.try_into()?,
             }),
             _ => Err(anyhow::Error::from(UnexpectedQueryParameters {
                 action: "ethereum::ContractDeploy",
@@ -280,7 +280,7 @@ impl IntoResponsePayload for ethereum::CallContract {
                 data,
                 gas_limit,
                 chain_id,
-                network: chain_id.into(),
+                network: chain_id.try_into()?,
                 min_block_timestamp,
             }),
             _ => Err(anyhow::Error::from(UnexpectedQueryParameters {
@@ -353,7 +353,7 @@ mod test {
             data: None,
             gas_limit: U256::from(1),
             chain_id,
-            network: chain_id.into(),
+            network: chain_id.try_into().unwrap(),
             min_block_timestamp: None,
         };
         let serialized = serde_json::to_string(&contract).unwrap();

cnd/src/http_api/action.rs

@@ -22,43 +22,46 @@ pub enum Network {
     Regtest,
     #[strum(serialize = "ropsten")]
     Ropsten,
-    #[strum(serialize = "unknown")]
-    Unknown,
 }
 
+#[derive(Debug, thiserror::Error)]
+#[error("chain with id {0} is unknown")]
+pub struct UnknownChainId(String);
+
 impl Network {
-    pub fn from_network_id(s: String) -> Self {
-        match s.as_str() {
+    pub fn from_chain_id(s: &str) -> Result<Self, UnknownChainId> {
+        Ok(match s {
             "1" => Network::Mainnet,
             "3" => Network::Ropsten,
             "17" => Network::Regtest,
-            _ => Network::Unknown,
-        }
+            _ => return Err(UnknownChainId(s.to_string())),
+        })
     }
 }
 
-impl From<ChainId> for Network {
-    fn from(chain: ChainId) -> Self {
-        Network::from_network_id(u32::from(chain).to_string())
+impl TryFrom<ChainId> for Network {
+    type Error = UnknownChainId;
+
+    fn try_from(value: ChainId) -> Result<Self, Self::Error> {
+        let value = u32::from(value).to_string();
+        Network::from_chain_id(value.as_str())
     }
 }
 
-impl TryFrom<Network> for ChainId {
-    type Error = ();
-
-    fn try_from(network: Network) -> Result<Self, ()> {
+impl From<Network> for ChainId {
+    fn from(network: Network) -> Self {
         match network {
-            Network::Mainnet => Ok(ChainId::mainnet()),
-            Network::Regtest => Ok(ChainId::regtest()),
-            Network::Ropsten => Ok(ChainId::ropsten()),
-            Network::Unknown => Err(()),
+            Network::Mainnet => ChainId::mainnet(),
+            Network::Regtest => ChainId::regtest(),
+            Network::Ropsten => ChainId::ropsten(),
         }
     }
 }
 
 #[cfg(test)]
 mod test {
     use super::*;
+    use spectral::prelude::*;
     use std::fmt::Display;
 
     #[test]
@@ -74,22 +77,10 @@ mod test {
 
     #[test]
     fn from_version() {
-        assert_eq!(
-            Network::from_network_id(String::from("1")),
-            Network::Mainnet
-        );
-        assert_eq!(
-            Network::from_network_id(String::from("3")),
-            Network::Ropsten
-        );
-        assert_eq!(
-            Network::from_network_id(String::from("17")),
-            Network::Regtest
-        );
-        assert_eq!(
-            Network::from_network_id(String::from("-1")),
-            Network::Unknown
-        );
+        assert_that(&Network::from_chain_id("1")).is_ok_containing(Network::Mainnet);
+        assert_that(&Network::from_chain_id("3")).is_ok_containing(Network::Ropsten);
+        assert_that(&Network::from_chain_id("17")).is_ok_containing(Network::Regtest);
+        assert_that(&Network::from_chain_id("-1")).is_err();
     }
 
     fn assert_display<T: Display>(_t: T) {}

cnd/src/http_api/asset.rs

@@ -3,35 +3,6 @@ macro_rules! _count {
     ($x:tt $($xs:tt)*) => (1usize + _count!($($xs)*));
 }
 
-macro_rules! impl_serialize_type_name_with_fields {
-    ($type:ty $(:= $name:tt)? { $($field_name:tt $(=> $field_value:ident)?),* }) => {
-        impl Serialize for Http<$type> {
-
-            fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-            where
-                S: Serializer,
-            {
-                let params = _count!($($name)*);
-                let mut state = serializer.serialize_struct("", 1 + params)?;
-
-                #[allow(unused_variables)]
-                let name = stringify!($type).to_lowercase();
-                #[allow(unused_variables)]
-                let name = name.as_str();
-                $(let name = $name;)?
-
-                state.serialize_field("name", name)?;
-
-                $(
-                  state.serialize_field($field_name, &(self.0)$(.$field_value)?)?;
-                )*
-
-                state.end()
-            }
-        }
-    };
-}
-
 macro_rules! impl_serialize_type_with_fields {
     ($type:ty { $($field_name:tt $(=> $field_value:ident)?),* }) => {
         impl Serialize for Http<$type> {