search

interledger / interledger-rs

An easy-to-use, high-performance Interledger implementation written in Rust
http://interledger.rs
Other
201 stars 70 forks source link

Overuse of interledger_packet::errors::ParseError #708

Closed koivunej closed 3 years ago

koivunej commented 3 years ago

The most popular case is ParseError::InvalidPacket which requires allocation of a String, which is most often a static string but might be result of a format!.

Some variants like ParseError::WrongType are never constructed, instead ParseError::InvalidPacket is used:

Going over through all calls to ParseError::InvalidPacket and replacing them with actual variants might be the way forward. Since ParseError is also used from other places (ccp, stream), and in relation to #704 it might be better to separate OER errors from the interledger-packet errors. This coupling currently forces the #704 case to be quite invisible since when decryption fails:

  1. It is reported as a String https://github.com/interledger-rs/interledger-rs/blob/8820e8de7f5f14644032346fce9e58ab4c20d1fd/crates/interledger-stream/src/packet.rs#L143-L144
  2. The string gets immediatedly dropped on the next callsite https://github.com/interledger-rs/interledger-rs/blob/8820e8de7f5f14644032346fce9e58ab4c20d1fd/crates/interledger-stream/src/server.rs#L285-L286

If instead interledger-stream would use smaller custom enum, like below, it would allow more clear separation for "bad packet" and "failed to decrypt".

enum InvalidPacket {
    // this assumes that interledger_packet::oer::* functionality would
    // report EncodingError instead of ParseError
    Encoding(interledger_packet::EncodingError),
    FailedToDecrypt,
}

Tagging this as breaking, as at least the interledger-packet apis are public. Inside {ccp,stream} the errors should not leak, and changes should only affect the crate-internal API.