Pass full data to callback

[Art3miX]

Related to: #3.

Still a draft because we still have 1 question to answer regarding this (related to this) :
How do we pass data to callback?

We basically have 2 callback types, execute type and query type, in this PR I handle both of those in a single callback type, but this creates an issue where you need to parse the returned data based on what you expect (either a query or an execution) or you need to match based on the parsing result rather then on a type

The PR works, but it feels almost unnatural:

polytone/contracts/proxy/src/contract.rs

Lines 76 to 79 in 01d2688

	let collector: Vec<Binary> = collector
	.into_iter()
	.map(|res| to_binary(&res.unwrap()))
	.collect::<Result<Vec<Binary>, StdError>>()?;

Because the query msg return vector of binary, we also need to turn SubMsgResponse into a binary (to fulfil the callback single type), and then we need to do several parsing steps to get our actual data.

I suggest we move into 2 callback types corresponding to the Rx 2 msg types (query and execute), that way we can remove the need to encode SubMsgResponse into binary, and it will be much easier to parse the needed data, and will make the callback handler much easier to write/read.

We should also extend tester contract to include some callback handler logic, where you do X when you get a query, and do Y when you get an execute callback.

Hope it makes sense.

[0xekez]

this looks good, and good thoughts. what do you think is better: making the callback data an enum with a query and execute variant, or having the caller unmarshal based on the initiation action?

my thought initially was that the type inside could be distinguished based on the initiator_data field.

[Art3miX]

How about Callback::SuccessExecute() and Callback::SuccessQuery() ?

I don't like the 2 success cases in one enum, but it seems to be the easiest way of implementing it, but also understanding it

pub enum Callback {
    SuccessQuery(Vec<Binary>),
    SuccessExecute(Vec<SubMsgResponse>),
    Error(String),
}

match callback {
    Callback::SuccessExecute{ execute_responses } => handle_execute_responses(execute_responses),
    Callback::SuccessQuery{ query_responses } => handle_query_responses(query_responses),
    Callback::Error{ err } => handle_error(err)
}

match callback {
    Callback::SuccessExecute{ execute_responses } => handle_execute_responses(execute_responses),
    Callback::SuccessQuery{ _} => handle_error("We don't expect queries!"),
    Callback::Error{ err } => handle_error(err)
}

I kind of like it after seeing it.
We can remove the "Success" word and instead of seeing it as a result, see it just as different cases, thoughts?

[0xekez]

dang yeah that looks good. i like that you wrote out how a contract author would use it. let's drop the Success prefix and go with this!

[0xekez]

@Art3miX what do you think about this idea:

what if we turn this:

polytone/packages/polytone/src/callback.rs

Lines 21 to 39 in cddf154

	pub struct CallbackMessage {
	/// Initaitor on the controller chain.
	pub initiator: Addr,
	/// Message sent by the initaitor. This _must_ be base64 encoded
	/// or execution will fail.
	pub initiator_msg: Binary,
	/// Data from the host chain.
	pub result: Callback,
	}
	#[cw_serde]
	pub enum Callback {
	/// Data returned from the host chain. Index n corresponds to the
	/// result of executing the nth message/query.
	Success(Vec<Option<Binary>>),
	/// The first error that occured while executing the requested
	/// messages/queries.
	Error(String),
	}

into this:

pub enum CallbackMsg {
   Execute {
     result: Vec<SubMsgResponse>,
     /* other fields */
   },
   Query {
     result: Vec<Binary>,
     /* same other fields as execute variant */
   }
}

I notice that what you wrote out is already matching on the variant, and this way if you only expected execute responses, you wouldn't need any extra code for queries (could just omit the variant).

[Art3miX]

I might be missing something, but where error is handled? also CallbackMsg is the type we send to the callback, but the Callback is the type we use in return data, so we still need 2 types here, no?

How about:

pub enum ExecuteResult {
  Success(Vec<SubMsgResponse>), 
  Error(String), 
}

pub enum QueryResult {
  Success(Vec<Binary>), 
  Error(String), 
}

pub enum Callback {
   Execute (ExecuteResult),
   Query (QueryResult)
}

That way we can implement .unwrap_execute() and .unwrap_query(), and return result of that

// res == Vec<SubMsgResponse>
// or throws an error if not execute (a query) or if executeResult is error
let res = some_callback.unwrap_execute()?; 

AND if a contract wants to implement both of them in a single callback, then we can implement .unwrap() on both of our results

match some_callback {
  Execute ( result: ExecuteResult ) => { let res = result.unwrap()?; } // res == Vec<SubMsgResponse> or throws the error in Error
  Query ( result: QueryResult ) => { let res = result.unwrap()?; } // res == Vec<Binary> or throws the error in Error
}

Maybe unwrap is the wrong word here, but you get the point

[0xekez]

I might be missing something, but where error is handled?

ah, you're so right. my design gets too complex if we need a variant for both query and execute errors. thanks.

what would you think of a slight tweak of yours to make the callback data generic? it's also possible this is more advanced rust than we want, so whatever you think.

/// Executed on the callback receiver upon message completion. When
/// being executed, the message will be tagged with "callback":
///
/// ```json
/// {"callback": {
///       "initiator": ...,
///       "initiator_msg": ...,
///       "result": ...,
/// }}
/// ```
#[cw_serde]
pub struct CallbackMessage {
    /// Initaitor on the controller chain.
    pub initiator: Addr,
    /// Message sent by the initaitor. This _must_ be base64 encoded
    /// or execution will fail.
    pub initiator_msg: Binary,
    /// Data from the host chain.
    pub result: Callback,
}

pub enum Callback {
    /// Callback data from a query.
    Query(CallbackData<QueryResponse>),
    /// Callback data from message execution.
    Execute(CallbackData<SubMsgResult>),
}

#[cw_serde]
pub enum CallbackData<T> {
    /// Data returned from the host chain. Index n corresponds to the
    /// result of executing the nth message/query.
    Success(T),
    /// The first error that occured while executing the requested
    /// messages/queries.
    Error(String),
}

[0xekez]

pub enum Callback {
SuccessQuery(Vec),
SuccessExecute(Vec),
Error(String),
}

I think I prefer the design that we've come up to this one because the grouping is more logical. For a contract that does both execution and queries, making the error its own variant flattens the distinction between an execution failure and query failure.

[Art3miX]

Here are 2 things I didn't liked too much, but just couldn't think of a simpler way of implementing it.

Waiting for other PRs so the conflicts are gonna be easier.

[Art3miX]

There are some errors where we don't know what the type of the callback is, so created this type to match against before executing the callback

[0xekez]

two thoughts about this type:

1. i don't think this needs to be public as you take it back apart in unmarshal_ack?
2. spelling:InternalError

also, a small test for you:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_forge_success() {
        let err = "{\"execute\":{\"success\":[]}}".to_string();
        let ack = ack_fail(err.clone());

        // ack is now base64 of `"{\"execute\":{\"success\":[]}}"`
        // whereas a real ack looks like
        // `{"execute":{"success":[]}}`. note the string escaping and
        // opening/closing quotes.
        let result = unmarshal_ack(&IbcAcknowledgement::new(ack), RequestType::Execute);
        assert_eq!(result, Ack::Execute(CallbackData::Error(err)))
    }
}

[Art3miX]

Created this type to we can figure out what type of callback we need to do.

[0xekez]

maybe a little docstring?

/// Disambiguates between a callback for remote message execution and
/// queries.

[0xekez]

this is really good!

last thing to write a test for remote contract instantiation and parsing the result in the response. it might be nice to do this by modifying the polytone-tester contract and running a whole "instantiate on remote chain and parse response for address in callback".

i like how you used the RequestType and InternalError types in this PR. both do their job well internally and don't pollute external APIs.

[0xekez]

two thoughts about this type:

1. i don't think this needs to be public as you take it back apart in unmarshal_ack?
2. spelling:InternalError

also, a small test for you:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_forge_success() {
        let err = "{\"execute\":{\"success\":[]}}".to_string();
        let ack = ack_fail(err.clone());

        // ack is now base64 of `"{\"execute\":{\"success\":[]}}"`
        // whereas a real ack looks like
        // `{"execute":{"success":[]}}`. note the string escaping and
        // opening/closing quotes.
        let result = unmarshal_ack(&IbcAcknowledgement::new(ack), RequestType::Execute);
        assert_eq!(result, Ack::Execute(CallbackData::Error(err)))
    }
}

[0xekez]

maybe a little docstring?

/// Disambiguates between a callback for remote message execution and
/// queries.

[Art3miX]

Fixed review, fixed conflicts, added test

[0xekez]

done in #33