interledger-service API review for v1.0-rc

[gakonst]

interledger-service public items marked with x are documented. Non-exported items are not taken into account. Follows format from #561.

Part of #557.

• username (struct)
• trace(module): Does this need to be exported? It just contains implementations to the service traits for Instrumented
• Account (trait)
  • Should we document each function's expected output? Their names are pretty self-explanatory.
• IncomingRequest (struct)
  • Maybe we should add comments to each field of the struct as well?
• OutgoingRequest (struct)
  • Maybe we should add comments to each field of the struct as well?
• IncomingService (trait):
  • Should we document the expected behavior of handle_request?
• OutgoingService (trait):
  • Should we document the expected behavior of send_request?
• BoxedIlpFuture (type)
• AccountStore (trait):
  • Should we document each function's expected output? Their names are pretty self-explanatory.
• incoming_service_fn / outgoing_service_fn
• ServiceFn
• WrappedService:
  • (nit) we can rename the generic parameters for the wrap_incoming and wrap_outgoing functions to be I and O for consistency, instead of IO
• AddressStore:
  • Should be filled to explain why this functionality is needed/used, short explanation: when a parent account is added the node's address must be updated to reflect the address assigned to it by the parent

[emschwartz]

This is going to have some serious breaking changes (or potentially be removed) based on #434 #142 #85

[bstrie]

If we did replace the services with Tower, I imagine we'd still want this crate around to provide IncomingRequest, OutgoingRequest, and Account.

[bstrie]

Should AddressStore be defined in interledger-packet, where Address is defined?

[emschwartz]

Should AddressStore be defined in interledger-packet, where Address is defined?

No. interledger-packet is just the (de)serialization format.

[gakonst]

Maybe we can remove the mut requirement from services, as @kincaidoneil pointed out:

Alternatively... why does handle_request on the service interface require a mutable borrow? Could we change it to just &self?

[bstrie]

If we're not sure why that function is &mut self then I'd say it's worth a try to remove it, if only to see whether it causes any problems (assuming that we don't actually need mutability anywhere, then it should be entirely painless to go from &mut self to &self). But I forget the broader context of the conversation, was there something else that we were hoping to gain from this? Something like, say, loosening up some other bounds elsewhere?

[bstrie]

• if the trace module doesn't need to be exported, then don't export it. Reduce public surface area.
• Account/AccountStore: if the names are self-explanatory then don't worry about it.
• IncomingRequest/OutgoingRequest: comments on fields sounds good to me
• IncomingService/OutgoingService: if the behavior isn't self-explanatory, then it should be documented
• WrappedService: rename the parameters for consistency (though I seem to remember that this was already done?)

[gakonst]

• trace is no longer exported
• account / account-store have been thoroughly documented
• comments in Request fields have been added
• Comments on Service types and their functions have been added
• WrappedService params are in a good spot