Skip to content

feat: use read_state rather than request_status#70

Merged
84 commits merged intonextfrom
es-1139-resp-auth-use-read-state
Nov 18, 2020
Merged

feat: use read_state rather than request_status#70
84 commits merged intonextfrom
es-1139-resp-auth-use-read-state

Conversation

@ghost
Copy link

@ghost ghost commented Oct 27, 2020

Use read_state rather than request_status.

  • Leaves Agent.request_status_raw in place, but reimplemented in terms of read_state_raw.
  • The read_state_raw method is not public because HashTree and Certificate are not public.
  • Updated request id calculation to account for arrays.
  • Moved fields from AgentError::HttpError to a separate structure in order to be able to implement Display and Debug for only that type.
  • Removed support for maps from request id calculation. This support wasn't that useful because the value types in the rust map would have to be homogenous, and they typically contain principals and strings at a minimum. Also, the map support was broken anyway, always returning H("").
  • Removed the call and call_rejected unit tests, because these cases are covered by integration tests and it didn't seem worth it to update them.

See also companion PR sdk #1171

@github-actions
Copy link

Netlify deployed agent-rust as draft

Link: https://5f979af411f4766877900695--agent-rust.netlify.app

@github-actions
Copy link

Netlify deployed agent-rust as draft

Link: https://5f979faf07d3508472ad5e61--agent-rust.netlify.app

@github-actions
Copy link

Netlify deployed agent-rust as draft

Link: https://5f97a360521fc26c7e9473fb--agent-rust.netlify.app

@nomeata
Copy link
Contributor

nomeata commented Oct 27, 2020

I think the current problem is that the agent has not implemented calculating the request id of requests with fields that are arrays (sequences). I added a test case that shows the problem:

~/dfinity/agent-rs $ cargo test -- request_id
   Compiling ic-agent v0.1.0 (/home/jojo/dfinity/agent-rs/ic-agent)
   Compiling ic-utils v0.1.0 (/home/jojo/dfinity/agent-rs/ic-utils)
   Compiling icx v0.1.0 (/home/jojo/dfinity/agent-rs/icx)
   Compiling ref-tests v0.0.0 (/home/jojo/dfinity/agent-rs/ref-tests)
    Finished test [unoptimized + debuginfo] target(s) in 9.12s
     Running target/debug/deps/ic_agent-c1b673971304d262

running 3 tests
test request_id::tests::public_spec_example ... ok
test request_id::tests::public_spec_example_api_client ... ok
test request_id::tests::array_example ... FAILED

failures:

---- request_id::tests::array_example stdout ----
thread 'request_id::tests::array_example' panicked at 'assertion failed: `(left == right)`
  left: `"f673c7985b9bb8ff7e7340951d297b15e0f0a8893855a38efef59da35a421fc4"`,
 right: `"f3a1b98b84b331f8d536d9509c8ec5116189acdeb9a0974d9d8c26cdacca65d5"`', ic-agent/src/request_id/mod.rs:742:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    request_id::tests::array_example

test result: FAILED. 2 passed; 1 failed; 0 ignored; 0 measured; 9 filtered out

error: test failed, to rerun pass '-p ic-agent --lib'

Also, the ic-ref repo provides an ic-request-id tool that you can use to calcualte expected request ids. Pass it a file with the CBOR encoding of a full request (with envelope), and it will tell you the request id that it expects. For your request in the PR description, I get

~/dfinity/ic-ref/impl $ cat /tmp/foo.hex 
d9 d9 f7 a3 67 63 6f 6e 74 65 6e 74 a4 6c 72 65 71 75 65 73 74 5f 74 79 70 65 6a 72 65 61 64 5f 73 74 61 74 65 6e 69 6e 67 72 65 73 73 5f 65 78 70 69 72 79 1b 16 41 bd 1a c3 12 16 08 66 73 65 6e 64 65 72 58 1d 4d 0d 0f 96 1d 14 be b2 98 aa bb cd 20 01 00 82 19 79 8f 64 02 e5 40 0c 48 eb 57 e3 02 65 70 61 74 68 73 81 82 4e 72 65 71 75 65 73 74 5f 73 74 61 74 75 73 58 20 b1 04 99 47 92 31 7b 94 22 97 57 fb 4a 17 ef ca 74 cd f8 b4 89 ff 35 ed 65 90 59 f7 cd 5e bd 03 6d 73 65 6e 64 65 72 5f 70 75 62 6b 65 79 58 20 50 b7 ba 78 3e 18 47 d8 35 79 e9 f0 b4 91 02 a2 c4 f3 0f ea e8 20 0c f6 7a 41 ee b7 d8 ae e8 b4 6a 73 65 6e 64 65 72 5f 73 69 67 58 40 68 22 66 0b c8 a5 34 15 2a 18 47 8d 30 e3 c9 35 d4 33 7a 21 e7 96 2a 84 ca 90 a0 27 fc 01 a6 65 97 6a bc 43 6b 3a c1 1d 95 60 7e e4 21 f3 8d 2c 1e 0b 49 c5 aa 5c 16 bd 47 20 e8 81 54 10 f3 0a
~/dfinity/ic-ref/impl $ xxd -r -ps /tmp/foo.hex > /tmp/foo.cbor
~/dfinity/ic-ref/impl $ file /tmp/foo.cbor 
/tmp/foo.cbor: Concise Binary Object Representation (CBOR) container (array) (map)
~/dfinity/ic-ref/impl $ nix run -f . -c ic-request-id /tmp/foo.cbor 
6f60f01e84ddc6c2d7c93fdd30315684f2d434067ebb707c2500d14befd39144

I will also make ic-ref return the expected request id if signature verification fails, for easier debugging, it now returns (in the body of the 4xx response)

        signature verification failed
        Expected request id: 0xf790fc25d1be63e75015ca04c5a90f145096f88de98d4e5921581db063150099
        Public Key:          0x9686a604230e16638534286d8c50fb059846505d57fba9c7549c8f0ba34dbeec
        Signature:           0xc8e3e6d8ffe28ae3322fc794e2e74a36f8977cefcabfb1be0b2b3df01fd5b9d5a0c58380a86906401429e4a3f7cc9509ad2aa6d54448fe0d4a056f7ca8ad7607

@github-actions
Copy link

Netlify deployed agent-rust as draft

Link: https://5f992b4f4a01644d85425410--agent-rust.netlify.app

@nomeata
Copy link
Contributor

nomeata commented Oct 28, 2020

Pushed now, sorry

@github-actions
Copy link

Netlify deployed agent-rust as draft

Link: https://5f99e7dff83d3300c3b35da1--agent-rust.netlify.app

@ghost ghost marked this pull request as ready for review November 12, 2020 23:24
@nomeata
Copy link
Contributor

nomeata commented Nov 18, 2020

Just curious what to watch: what is the relation between this PR and #82?

@ghost
Copy link
Author

ghost commented Nov 18, 2020

#82 adds certificate verification, root key fetching, delegations

let sender = match &request {
SyncContent::QueryRequest { sender, .. } => sender,
SyncContent::ReadStateRequest { sender, .. } => sender,
SyncContent::RequestStatusRequest { .. } => &anonymous,
Copy link
Author

Choose a reason for hiding this comment

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

Let's remove the RequestStatusRequest type

"reject_message".into(),
];

let reject_code = match certificate.tree.lookup_path(&path_reject_code) {
Copy link
Author

Choose a reason for hiding this comment

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

Let's DRY this lookup result handling

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

LGTM with the above two comments.

@ghost ghost merged commit 9c5ff9a into next Nov 18, 2020
@ghost ghost deleted the es-1139-resp-auth-use-read-state branch November 18, 2020 20:41
ghost pushed a commit that referenced this pull request Nov 18, 2020
- request id calculation accounts for arrays
- HttpErrorPayload for text/plain decoding
This pull request was closed.
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.

2 participants