interledger-packet API review for v1.0-rc

[bstrie]

As per #557, it's time to start thinking about stable releases of the crates in this repo. As a demonstration of the sort of process that might entail, this issue exists to discuss the public API of interledger-packet (i.e. the only one of our crates that doesn't depend on any of our others, thus the natural starting point for stabilization).

Below we will list every public item exported by interledger-packet, including items marked as #[doc(hidden)], because just because something is hidden from the docs doesn't mean it wouldn't be stabilized by a 1.0 release (whether or not these items ought to remain hidden by the docs is worth discussion as well).

Beneath each item I will list any open questions that come to mind regarding that item. Please feel free to edit this text to add in your own questions. Resolved questions should have their checkbox checked. Items with no unresolved questions should have their checkbox checked.

For now let's not bother noting which items need documentation; it should be taken as given that all public items will need to be at least cursorily documented before being stabilized.

For brevity I won't be including full function signatures in this list, although obviously this is also important to keep in mind when considering stabilizing an item. Please review the signatures of all items yourself (which can be done easily via cd interledger-packet; cargo doc --open). Nor will the full bevy of trait implementations/implementors be considered.

• oer (module)
  • Does this submodule need to exist (could its types live at the root level)? (Conspicuously, nowhere do we seem to define what "OER" means.)
• oer::BufOerExt (trait)
  • Why does interledger-btp define its own BufOerExt trait? Does that imply some deficiency of this one that could be addressed?
  • Why is this implemented for &[u8] when MutBufOerExt is instead implemented for B: BufMut? Should this be implemented for B: Buf?
• oer::BufOerExt::peek_var_octet_string (fn)
• oer::BufOerExt::read_var_octet_string (fn)
• oer::BufOerExt::skip (fn)
• oer::BufOerExt::skip_var_octet_string (fn)
• oer::BufOerExt::read_var_octet_string_length (fn)
  • Why is this function marked as doc(hidden)? Does it need to be public?
• oer::BufOerExt::read_var_unit (fn)
• oer::MutBufOerExt (trait)
  • As above, why does interledger-btp define its own version of this?
• oer::MutBufOerExt::put_var_octet_string (fn)
• oer::MutBufOerExt::put_var_octet_string_length (fn)
  • Why is this function marked as doc(hidden)? Does it need to be public?
• oer::extract_var_octet_string (fn)
  • Why not define this as a method on MutBufOerExt?
• oer::predict_var_octet_string (fn)
• Address (struct)
• Address::len (fn)
• Address::to_bytes (fn)
• Address::new_unchecked (fn)
• Address::segments (fn)
• Address::scheme (fn)
• Address::with_suffix (fn)
• ErrorCode (struct)
• ErrorCode::new (fn)
  • Benign, but does this provide any value over simply using the raw constructor?
• ErrorCode::class (fn)
• (lots of seemingly-uninteresting inherent associated consts, one for each potential error code)
• Fulfill (struct)
• Fulfill::fulfillment (fn)
• Fulfill::data (fn)
• Fulfill::into_data (fn)
  • impl From<Fulfill> for BytesMut?
• FulfillBuilder (struct)
  • Does this need to exist, given that this only has a single method, build? Could it be eliminated by simply moving FulfillBuilder::build to Fulfill::new?
• MaxPacketAmountDetails (struct)
• MaxPacketAmountDetails::new (fn)
• MaxPacketAmountDetails::from_bytes (fn)
  • impl TryFrom<&[u8]> for MaxPacketAmountDetails?
• MaxPacketAmountDetails::to_bytes (fn)
  • impl From<MaxPacketAmountDetails> for [u8; 16]?
• MaxPacketAmountDetails::amount_received (fn)
• MaxPacketAmountDetails::max_amount (fn)
• Prepare (struct)
• Prepare::amount (fn)
• Prepare::set_amount (fn)
• Prepare::expires_at (fn)
• Prepare::set_expires_at (fn)
• Prepare::execution_condition (fn)
• Prepare::destination (fn)
• Prepare::data (fn)
• Prepare::into_data (fn)
  • impl From<Prepare> for BytesMut?
• PrepareBuilder (struct)
  • Like FulfillBuilder, replace in favor of a new function? Potential downside: unlike FulfillBuilder this struct has more than two fields, and a new constructor doesn't allow named fields as a struct constructor does.
• Reject (struct)
• Reject::code (fn)
• Reject::triggered_by (fn)
• Reject::message (fn)
• Reject::data (fn)
• Reject::into_data (fn)
  • impl From<Reject> for BytesMut?
• RejectBuilder (struct)
  • Same treatment as PrepareBuilder?
• AddressError (enum)
  • Worth folding into ParseError?
• ErrorClass (enum)
• Packet (enum)
  • Redundant with PacketType?
• PacketType (enum)
  • Redundant with Packet?
• ParseError (enum)

[bstrie]

Paging @emschwartz , @gakonst , @dora-gt to see if anyone has any thoughts about this.

(Reminder that this is also a trial of the process we might use to stabilize the other crates as well, let me know if you have any feedback on the process itself.)

[gakonst]

Regarding OER: https://interledger.org/rfcs/0030-notes-on-oer-encoding/

[emschwartz]

oer - Does this submodule need to exist (could its types live at the root level)?

For a while I thought we might want to either make this private or move it to a completely separate crate. However, it might make sense to just keep it the way it is. OER is an encoding format that could be implemented on its own in a separate crate (ideally as a full serde-compatible format) but for now it's primarily used in places where the ILP packet is also used so it may be fine to keep them together.

Why does interledger-btp define its own BufOerExt trait? Does that imply some deficiencty of this one that could be addressed?

I think this is just duplication. Originally, the two implementations of OER were the same but the one in ilp-packet was changed when @sentientwaffle implemented zero-copy packet parsing and in-place modification. The only reason we didn't touch BTP was that there has long been a lingering question about whether we would deprecate BTP, so it didn't seem worth modifying. All of this said, the OER stuff should not be exported from the interledger-btp crate.

ErrorCode::new (fn) - Benign, but does this provide any value over simply using the raw constructor?

No opinion

Fulfill::into_data (fn) - impl From for BytesMut?

No. This should stay the way it is. into_data consumes the packet but returns only the data portion of the packet, so it's different than just converting between formats

PrepareBuilder (struct) - Like FulfillBuilder, replace in favor of a new function?

I think keeping the builder is better, precisely because it has more fields and it seems cleaner when constructing it to have the fields named.

AddressError (enum)

We may want to change this to remove the dependency on quick_error, in favor of whatever the best practice is on error handling these days.

AddressError (enum) - Worth folding into ParseError?

No opinion

PacketType (enum) - Redundant with Packet?

PacketType is the numeric identifier used in the binary packet and defined in the ILP protocol spec. This should definitely be kept, though it may not be important to make it public since it's primarily used for (de)serialization.

Packet (enum)

Packet is the wrapper around the three packet structs. It isn't used anywhere in the codebase right now so it may be worth just getting rid of it. (There are places where the response can either be a Fulfill or Reject, so we could think about exporting a Result type that would be Result<Fulfill, Reject>, but there aren't many or any places where you'd expect a Prepare, Fulfill, or Reject. The Result type would be used all over the place when we move to futures 0.3, because the Future<Item = Fulfill, Error = Reject> will be replaced with Future<Output = Result<Fulfill, Reject>>)

[bstrie]

• oer::BufOerExt::read_var_octet_string_length/oer::MutBufOerExt::put_var_octet_string_length: both should either have their doc(hidden) removed or be removed from the public interface
• should we get rid of Packet? Evan seemed to suggest it was unnecessary

[gakonst]

• We shouldn't get rid of Packet, Evan said that it's not used, but it's actually used in the BTP implementation. https://github.com/interledger-rs/interledger-rs/blob/master/crates/interledger-btp/src/service.rs#L72-L103
• I removed the doc(hidden) fromput_var_octet_string_length and replaced it with a comment