Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻️ Forward Response.data on Execute message on DataRequest to Proxy contract #73

Merged
merged 4 commits into from
Oct 17, 2023

Conversation

jamesondh
Copy link
Contributor

Motivation

Makes fetching dr_id in integration tests cleaner (well, it's still pretty ugly to parse it but at least we aren't searching through events). Enables the overlay node and other applications to fetch dr_id easier after posting a data request from the overlay node.

Also provides a blueprint for how callback values work once we implement the Balances contract.

Explanation of Changes

  • Use submessage instead of message on Proxy in PostDataRequest to return the value back to the contract (helpful docs: https://docs.cosmwasm.com/docs/smart-contracts/message/submessage/ )
  • Added reply entry point on Proxy contract to handle data payload from DataRequest
  • Updated integration tests to use this to get the dr_id after posting a data request instead of looping through the events

Testing

See above

Related PRs and Issues

Closes #68

@jamesondh jamesondh marked this pull request as ready for review September 28, 2023 20:50
@jamesondh jamesondh requested a review from a team September 28, 2023 20:50
Copy link
Member

@Thomasvdam Thomasvdam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor remarks, not that important given that it's 'just' for tests.

Comment on lines 182 to 193
// I don't know why protobuf adds 2 bytes to the beginning of the data payload but it does
let binary = &binary.to_vec()[2..];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels fragile? The examples linked in the docs don't seem to do anything like this, maybe we should follow those and not use the utility?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not mistaken, this comes from here, right?

packages/data-requests/src/data_request.rs, line 113:

            .set_data(to_binary(&posted_dr.dr_id)?)

The thing is that dr_id of type Hash which is an alias type of String.
So in reality, we are representing bytes as a string, and then we are sending those bytes as if they were strings.
Also, as a side note, Binary does expect bytes or base64-encoded strings. 😕

Maybe knowing that dr_id are indeed bytes, we should take that into consideration for its serialilzation and, even, benefit from it.

Here you have a snippet of a simple test to show what I mean:

#[test]
fn test_binary_encoding() {
    // Hash example (sha256 of "data", not that it really matters!)
    let hash_string = "3a6eb0790f39ac87c94f3856b2dd2c5d110e6811602261a9a923d3bb23adc8b7";
    // Bytes
    let hash_bytes = hex::decode(hash_string).unwrap();
    // Encoded oofline to avoid adding a  new dependency
    let hash_base64 =
        "M2E2ZWIwNzkwZjM5YWM4N2M5NGYzODU2YjJkZDJjNWQxMTBlNjgxMTYwMjI2MWE5YTkyM2QzYmIyM2FkYzhiNw==";

    // Simple string
    let t1 = to_binary(hash_string).unwrap();
    // 
    // data in base64
    let t2 = cosmwasm_std::Binary::from_base64(hash_base64).unwrap();
    let t3 = cosmwasm_std::Binary::from(hash_bytes);

    println!("binary 1: {}", hex::encode(&t1));
    println!("binary 2: {}", hex::encode(&t2));
    println!("binary 3: {}", hex::encode(&t3));

    println!(
        "recovered text 1: {}",
        String::from_utf8(t1.to_vec()).unwrap()
    );
    println!(
        "recovered text 2: {}",
        String::from_utf8(t2.to_vec()).unwrap()
    );
    println!("recovered text 2 base64: {}", t2);
    println!("recovered text 3: {}", hex::encode(t3));
}

The output is as follows:

binary 1: 223361366562303739306633396163383763393466333835366232646432633564313130653638313136303232363161396139323364336262323361646338623722
binary 2: 33613665623037393066333961633837633934663338353662326464326335643131306536383131363032323631613961393233643362623233616463386237
binary 3: 3a6eb0790f39ac87c94f3856b2dd2c5d110e6811602261a9a923d3bb23adc8b7
recovered text 1: "3a6eb0790f39ac87c94f3856b2dd2c5d110e6811602261a9a923d3bb23adc8b7"
recovered text 2: 3a6eb0790f39ac87c94f3856b2dd2c5d110e6811602261a9a923d3bb23adc8b7
recovered text 2 base64: M2E2ZWIwNzkwZjM5YWM4N2M5NGYzODU2YjJkZDJjNWQxMTBlNjgxMTYwMjI2MWE5YTkyM2QzYmIyM2FkYzhiNw==
recovered text 3: 3a6eb0790f39ac87c94f3856b2dd2c5d110e6811602261a9a923d3bb23adc8b7

As you may see, it is more efficient to serialize and deserialize as bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented serializing and deserializing dr_id as bytes using hex decode/encode using your suggestion, along with a helper function from the cw_utils crate to remove the bytes prepended to the binary payload. Since Hash is currently implemented as a wrapped String it gets a bit ugly when prepending 0x, but this will be cleaned up in #65 .

Comment on lines 187 to 198
// remove first and last char (they are quotes)
let dr_id = &dr_id[1..dr_id.len() - 1];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine this is JSON serialisation so maybe we can just deserialise the bytes directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also from my previous comment, I believe that the quotes are because what Is how with my little test (more specifically t1).

recovered text 1: "3a6eb0790f39ac87c94f3856b2dd2c5d110e6811602261a9a923d3bb23adc8b7"

Copy link
Member

@mariocao mariocao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merging, let's discuss about the Binary decoding. :)

@jamesondh jamesondh force-pushed the refactor/forward-data-to-proxy branch 3 times, most recently from 3bb325a to 7b13f03 Compare October 12, 2023 19:04
@jamesondh jamesondh requested a review from mariocao October 13, 2023 21:15
@jamesondh jamesondh force-pushed the refactor/forward-data-to-proxy branch from b6448c3 to ce000d8 Compare October 17, 2023 15:36
@jamesondh jamesondh force-pushed the refactor/forward-data-to-proxy branch from ce000d8 to 87a2c92 Compare October 17, 2023 15:37
Copy link
Member

@mariocao mariocao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much better with the parse_execute_response_data! 🎆

@jamesondh jamesondh merged commit 87a2c92 into main Oct 17, 2023
4 checks passed
@jamesondh jamesondh deleted the refactor/forward-data-to-proxy branch October 17, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

♻️ Forward Response.data on Execute message on DataRequest to Proxy contract
3 participants