Add comments from quick code review

vcx/libvcx/src/api/connection.rs

@@ -10,17 +10,37 @@ use error::ToErrorCode;
 use error::connection::ConnectionError;
 use connection::{get_source_id, build_connection, build_connection_with_invite, connect, to_string, get_state, release, is_valid_handle, update_state, from_string, get_invite_details, delete_connection};
 
-/// Delete a Connection object and release its handle
+/// Delete a Connection object and release its handle.
+///
+/// <daniel>
+/// What effect does deleting the Connection object have on the state of the underlying conn that it
+///   manages, as perceived by the two parties? Does it send a signal to the other party that the
+///   conn is being closed? Does it remove the other party's verkey from the local wallet? Does
+///   it cause the verkey for the local DID to be set to 0 so the connection is unrecoverable?
+///
+/// Document the preconditions:
+///   - valid connection_handle
+///   - valid pointer to callback
+///
+/// If this function fails, what cleanup is necessary?
+///
+/// What happens if another thread is using the connection_handle while this function is called?
+///
+/// Is this function idempotent (calling it multiple times on the same handle is harmless)? It looks
+/// like the answer is No (assuming that is_valid_handle() will return false on something that's
+/// already closed). Should it be? Document, either way.
+/// </daniel>
 ///
 /// #Params
-/// command_handle: command handle to map callback to user context.
+/// command_handle: command handle to map callback to user context. <daniel>I suggest calling it a
+///   "caller context" rather than a "user context".</daniel>
 ///
-/// connection_handle: handle of the connection to delete.
+/// connection_handle: handle of the connection to delete. <daniel>Precondition: must be valid.</daniel>
 ///
-/// cb: Callback that provides feedback of the api call.
+/// cb: Callback that provides feedback of the api call. <daniel>Precondition: must be non-null.</daniel>
 ///
 /// #Returns
-/// Error code as a u32
+/// Error code as a u32 <daniel>List common errors that could occur, if we know what they are.</daniel>
 #[no_mangle]
 #[allow(unused_assignments)]
 pub extern fn vcx_connection_delete_connection(command_handle: u32,
@@ -51,17 +71,31 @@ pub extern fn vcx_connection_delete_connection(command_handle: u32,
     error::SUCCESS.code_num
 }
 
-/// -> Create a Connection object that provides a pairwise connection for an institution's user
+/// -> Create a Connection object that provides a pairwise connection for an institution's user.
+///
+/// <daniel_notes>
+/// Need to clarify ordering assumptions -- do we call this as a means of exchanging the connection
+///   request/response, or as a precondition to sending such messages?
+///
+/// If this function fails, what cleanup is necessary?
+///
+/// What happens if I call this function twice on the same source_id?
+/// </daniel_notes>
 ///
 /// #Params
 /// command_handle: command handle to map callback to user context.
 ///
-/// source_id: institution's personal identification for the user
+/// source_id: institution's personal identification for the user <daniel>Need link to some place
+///   that describes this notion in more detail -- why we're not using DIDs, what some examples
+///   of this sort of identifier might be, etc. Precondition: cannot be null. But what other
+///   differences are significant? For example, is whitespace? Case? Unicode normalized vs. unnormalized
+///   form?
+/// </daniel>
 ///
-/// cb: Callback that provides connection handle and error status of request
+/// cb: Callback that provides connection handle and error status of request<daniel>. Precondition: cannot be null.</daniel>
 ///
 /// #Returns
-/// Error code as a u32
+/// Error code as a u32 <daniel>List common errors that could occur, if we know what they are.</daniel>
 #[no_mangle]
 #[allow(unused_assignments)]
 pub extern fn vcx_connection_create(command_handle: u32,
@@ -91,18 +125,32 @@ pub extern fn vcx_connection_create(command_handle: u32,
 }
 
 /// Create a Connection object from the given invite_details that provides a pairwise connection.
+/// <daniel>Need to provide some more commentary here, such as: "This function is typically called
+/// by the party that *receives* a connection invitation, not the first mover that sends the invitation
+/// to begin with. It is the second step in the following workflow: <hyperlink to sequence diagram>."
+///
+/// What can go wrong, and how are such problems handled? For example, if invite_details are
+/// unsupported (bad version, malformed), do we reply with an error to the sender of the invite?
+/// What cleanup do we have to do on error?
+///
+/// Are there any timeouts on callbacks? If not, what happens if we get into an infinite wait for
+/// the connection to complete?
+///
+/// Is a long-running command interruptible by command handle?
+/// </daniel>
 ///
 /// #Params
 /// command_handle: command handle to map callback to user context.
 ///
 /// source_id: institution's personal identification for the user
 ///
 /// invite_details: Provided via the other end of the connection calling "vcx_connection_connect" or "vcx_connection_invite_details"
+/// <daniel>Provide an example here, inline in the comments, so people can see what it looks like.</daniel>
 ///
-/// cb: Callback that provides connection handle and error status of request
+/// cb: Callback that provides connection handle and error status of request<daniel>. Precondition: cannot be null.</daniel>
 ///
 /// #Returns
-/// Error code as a u32
+/// Error code as a u32 <daniel>List common errors that could occur, if we know what they are.</daniel>
 #[no_mangle]
 pub extern fn vcx_connection_create_with_invite(command_handle: u32,
                                                 source_id: *const c_char,
@@ -135,12 +183,24 @@ pub extern fn vcx_connection_create_with_invite(command_handle: u32,
 
 /// Establishes connection between institution and its user
 ///
+/// <daniel>
+/// This function should be renamed, or else we should not claim that it only operates between
+/// institution and user; hopefully, the latter. We want connections to work the same way between
+/// two people, or between institutions and other institutions, etc.
+///
+/// On failure, should I call vcx_connection_delete_connection()? Are (some) errors retryable?
+///
+/// What happens if I call this function on a connection that's already connected?
+/// </daniel>
+///
 /// #Params
 /// command_handle: command handle to map callback to user context.
 ///
 /// connection_handle: Connection handle that identifies connection object
 ///
 /// connection_options: Provides details indicating if the connection will be established by text or QR Code
+///   <daniel>Need to clarify that null/empty string is allowed, and what it will mean if that is used
+///   as the value.</daniel>
 ///
 /// # Examples connection_options -> "{"connection_type":"SMS","phone":"123"}" OR: "{"connection_type":"QR","phone":""}"
 ///
@@ -186,6 +246,9 @@ pub extern fn vcx_connection_connect(command_handle:u32,
                     Err(e) => {
                         warn!("vcx_connection_connect_cb(command_handle: {}, connection_handle: {}, rc: {}, details: {}), source_id: {:?}",
                               command_handle, connection_handle, error_string(0), "null", source_id);
+                        // <daniel>Explain why we would be reporting success here even though we did
+                        // not find invite details. Could be fine that we are--but that raised my
+                        // eyebrows when I read the code.</daniel>
                         cb(command_handle, error::SUCCESS.code_num, ptr::null_mut());
                     },
                 }

vcx/libvcx/src/api/credential.rs

@@ -14,6 +14,15 @@ use utils::threadpool::spawn;
 
 /// Retrieves Payment Info from a Credential
 ///
+/// <daniel>What does a response look like in each of these cases: credential is free; credential
+/// has a one-time payment to issuer; credential wants a payment on each use?
+///
+/// Also, is this a call about payment info for a *credential*, or a *credential definition*? If
+/// it's about a credential, is it one that's already been issued, or about one where issuance
+/// is under active negotiation? If it's a credential under active negotiation, why wouldn't we
+/// be using a credential definition rather than a credential object?
+/// </daniel>
+///
 /// #Params
 /// command_handle: command handle to map callback to user context.
 ///
@@ -27,7 +36,12 @@ use utils::threadpool::spawn;
 #[allow(unused_variables, unused_mut)]
 pub extern fn vcx_credential_get_payment_info(command_handle: u32,
                                                credential_handle: u32,
-                                               cb: Option<extern fn(xcommand_handle: u32, err: u32, *const c_char)>) -> u32 {
+                                               cb: Option<extern fn(xcommand_handle: u32, err: u32,
+                                               // <daniel>This parameter needs to be named. It is also
+                                               // probably the most important param to be documented,
+                                               // by providing some sample JSON? that shows what it
+                                               // might contain.</daniel>
+                                               *const c_char)>) -> u32 {
     check_useful_c_callback!(cb, error::INVALID_OPTION.code_num);
     spawn(move|| {
         match credential::get_payment_information(credential_handle) {
@@ -62,6 +76,13 @@ pub extern fn vcx_credential_get_payment_info(command_handle: u32,
 }
 /// Create a Credential object that requests and receives a credential for an institution
 ///
+/// <daniel>
+/// Need clarification here about what it means to "create a credential" with an offer.
+/// I'm guessing that we create a credential object to represent a *potential* credential, not an
+/// issued one. Is that true? If so, is this only called by issuers, or could holders use this
+/// function as well? Also, do we call this before or after we have a credential request in hand?
+/// </daniel>
+///
 /// #Params
 /// command_handle: command handle to map callback to user context.
 ///

vcx/libvcx/src/api/utils.rs

@@ -21,8 +21,28 @@ pub struct UpdateAgentInfo {
 /// Provision an agent in the agency, populate configuration and wallet for this agent.
 /// NOTE: for asynchronous call use vcx_agent_provision_async
 ///
+/// <daniel>
+/// Because this is a sync function, is it only used in testing and should be deprecated in
+/// production? If yes, say so. If no, then I worry about the quality of the error reporting
+/// mechanism here; we don't have any way to tell the caller what went wrong--only that the
+/// function failed.
+///
+/// What preconditions apply to this function? For example, is it only callable once the
+/// agency has been through some sort of initialization phase?
+///
+/// Which errors are retryable?
+///
+/// Is there anything to protect us from provisioning multiple agents for the same person?
+///
+/// If we have errors that are very serious (e.g., agency isn't functional; no agent can ever
+/// be provisioned), do we have a way to transition the agency into a degraded state? See
+/// this design spec, which we may need to work harder to follow:
+///   https://github.com/evernym/design/blob/master/blueprints/agency/app-lifecycle.md
+/// </daniel>
+///
 /// #Params
 /// config: configuration
+/// <daniel>Need an example of valid config here.</daniel>
 ///
 /// #Returns
 /// Configuration (wallet also populated), on error returns NULL
@@ -50,6 +70,9 @@ pub extern fn vcx_provision_agent(config: *const c_char) -> *mut c_char {
 /// Provision an agent in the agency, populate configuration and wallet for this agent.
 /// NOTE: for synchronous call use vcx_provision_agent
 ///
+/// <daniel>See my comments on the sync version of the func--especially the part about
+/// needing to communicate errors better.</daniel>
+///
 /// #Params
 /// command_handle: command handle to map callback to user context.
 ///
@@ -92,10 +115,20 @@ pub extern fn vcx_agent_provision_async(command_handle : u32,
 
 /// Update information on the agent (ie, comm method and type)
 ///
+/// <daniel>I assume this function has a precondition that the agent has to have been
+/// successfully provisioned? What happens if the agent has not only been provisioned, but also
+/// decommissioned or deleted?
+///
+/// How is this function protected for concurrency (if agent info is updated at the same
+/// time it is being looked up, do we have a problem? I'm assuming Rust race condition guarantees
+/// tied to object ownership protect us, but I'm just wanting to confirm.
+/// </daniel>
+///
 /// #Params
 /// command_handle: command handle to map callback to user context.
 ///
 /// json: updated configuration
+/// <daniel>Need example of valid JSON here.</daniel>
 ///
 /// cb: Callback that provides configuration or error status
 ///
@@ -147,6 +180,14 @@ pub extern fn vcx_agent_update_info(command_handle: u32,
 ///
 /// cb: Callback that provides the fee structure for the sovrin network
 ///
+/// <daniel>
+/// We are ignoring the time element here (assuming the question is always, "What are the fees
+/// at the current instant?". We may want to introduce a new param to account for time. Or not.
+/// If we don't, we should clarify that the question is always asked about now.
+///
+/// Need to provide sample string that communicates the fees info in the callback.
+/// </daniel>
+///
 /// #Returns
 /// Error code as a u32