Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't unwrap in constructor of `SwapDescription` #765

Closed
thomaseizinger opened this Issue Feb 18, 2019 · 1 comment

Comments

Projects
None yet
3 participants
@thomaseizinger
Copy link
Member

thomaseizinger commented Feb 18, 2019

Currently, we are unwrapping the constructor of SwapDescription after the to_http_ledger and to_http_asset call:

impl SwapDescription {
fn new<AL: Ledger, BL: Ledger, AA: Asset, BA: Asset>(
request: rfc003::messages::Request<AL, BL, AA, BA>,
) -> Self {
Self {
alpha_ledger: request.alpha_ledger.to_http_ledger().unwrap(),
beta_ledger: request.beta_ledger.to_http_ledger().unwrap(),
alpha_asset: request.alpha_asset.to_http_asset().unwrap(),
beta_asset: request.beta_asset.to_http_asset().unwrap(),
}
}
}

Even though the error is quite rare, we should not do this.

Passing on the error is tricky since there are different ones for HttpLedger and HttpAsset.
Some investigation on serialization uncovered that there might actually be a better way to achieve contextual serialization, which is the reason why HttpLedger and HttpAsset were introduced.

A different approach would be to create a generic wrapper struct Http<T> that is used in all payloads where a different serialization for a type is needed, for example:

#[derive(Clone, Debug, Deserialize, PartialEq)]
#[serde(deny_unknown_fields)]
pub struct SwapRequestBody<AL: Ledger, BL: Ledger, AA: Asset, BA: Asset, PartialIdentities> {
    alpha_asset: Http<AA>,
    beta_asset: Http<BA>,
    alpha_ledger: Http<AL>,
    beta_ledger: Http<BL>,
    alpha_expiry: Timestamp,
    beta_expiry: Timestamp,
    #[serde(flatten)]
    partial_identities: PartialIdentities,
    #[serde(with = "http_api::rfc003::socket_addr")]
    peer: SocketAddr,
}

Since Http<T> is a local type, one can impl Serialize for Http<Bitcoin> to define a serialization for this context. Doing so would remove the need for HttpLedger and HttpAsset which would also simply get rid of the code that contains the unwrap that is mentioned at the beginning of this ticket.

As part of doing this, you will probably need custom trait bounds for serde.

@luckysori

This comment has been minimized.

Copy link
Contributor

luckysori commented Feb 25, 2019

Resolved with #785.

@luckysori luckysori closed this Feb 25, 2019

@wafflebot wafflebot bot removed the review label Feb 25, 2019

@D4nte D4nte added this to the Sprint 9 🏝🏋️‍♀️ milestone Feb 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.