diff --git a/vcx/libvcx/src/api/connection.rs b/vcx/libvcx/src/api/connection.rs index 60ee07eb..cdd0a005 100644 --- a/vcx/libvcx/src/api/connection.rs +++ b/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. +/// +/// +/// 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. +/// /// /// #Params -/// command_handle: command handle to map callback to user context. +/// command_handle: command handle to map callback to user context. I suggest calling it a +/// "caller context" rather than a "user context". /// -/// connection_handle: handle of the connection to delete. +/// connection_handle: handle of the connection to delete. Precondition: must be valid. /// -/// cb: Callback that provides feedback of the api call. +/// cb: Callback that provides feedback of the api call. Precondition: must be non-null. /// /// #Returns -/// Error code as a u32 +/// Error code as a u32 List common errors that could occur, if we know what they are. #[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. +/// +/// +/// 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? +/// /// /// #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 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? +/// /// -/// cb: Callback that provides connection handle and error status of request +/// cb: Callback that provides connection handle and error status of request. Precondition: cannot be null. /// /// #Returns -/// Error code as a u32 +/// Error code as a u32 List common errors that could occur, if we know what they are. #[no_mangle] #[allow(unused_assignments)] pub extern fn vcx_connection_create(command_handle: u32, @@ -91,6 +125,19 @@ pub extern fn vcx_connection_create(command_handle: u32, } /// Create a Connection object from the given invite_details that provides a pairwise connection. +/// 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: ." +/// +/// 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? +/// /// /// #Params /// command_handle: command handle to map callback to user context. @@ -98,11 +145,12 @@ pub extern fn vcx_connection_create(command_handle: u32, /// 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" +/// Provide an example here, inline in the comments, so people can see what it looks like. /// -/// cb: Callback that provides connection handle and error status of request +/// cb: Callback that provides connection handle and error status of request. Precondition: cannot be null. /// /// #Returns -/// Error code as a u32 +/// Error code as a u32 List common errors that could occur, if we know what they are. #[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 /// +/// +/// 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? +/// +/// /// #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 +/// Need to clarify that null/empty string is allowed, and what it will mean if that is used +/// as the value. /// /// # 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); + // 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. cb(command_handle, error::SUCCESS.code_num, ptr::null_mut()); }, } diff --git a/vcx/libvcx/src/api/credential.rs b/vcx/libvcx/src/api/credential.rs index 9972b816..eb686998 100644 --- a/vcx/libvcx/src/api/credential.rs +++ b/vcx/libvcx/src/api/credential.rs @@ -14,6 +14,15 @@ use utils::threadpool::spawn; /// Retrieves Payment Info from a Credential /// +/// 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? +/// +/// /// #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) -> u32 { + cb: OptionThis 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. + *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 /// +/// +/// 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? +/// +/// /// #Params /// command_handle: command handle to map callback to user context. /// diff --git a/vcx/libvcx/src/api/utils.rs b/vcx/libvcx/src/api/utils.rs index a0172b7e..ff1c255c 100644 --- a/vcx/libvcx/src/api/utils.rs +++ b/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 /// +/// +/// 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 +/// +/// /// #Params /// config: configuration +/// Need an example of valid config here. /// /// #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 /// +/// See my comments on the sync version of the func--especially the part about +/// needing to communicate errors better. +/// /// #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) /// +/// 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. +/// +/// /// #Params /// command_handle: command handle to map callback to user context. /// /// json: updated configuration +/// Need example of valid JSON here. /// /// 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 /// +/// +/// 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. +/// +/// /// #Returns /// Error code as a u32