Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add methods param for RPC state_traceBlock #9642

Merged
6 commits merged into from
Aug 30, 2021
Merged

Add methods param for RPC state_traceBlock #9642

6 commits merged into from
Aug 30, 2021

Conversation

koushiro
Copy link
Contributor

@koushiro koushiro commented Aug 28, 2021

Signed-off-by: koushiro [email protected]

Summary

Add methods param for RPC state_traceBlock to fetch all changed storages for a given block.

Close #9632

Example

curl \
  -H "Content-Type: application/json" \
  -d '{"id":1, "jsonrpc":"2.0", "method": "state_traceBlock", "params": ["0x0268dff7aa545d493e4640871bcdbb43a09240e5fb3efb91d9900782cabaed35", "f0c365c3cf59d671eb72da0e7a4113c4", "Put"]}' \
  http://localhost:9933
{
  "jsonrpc": "2.0",
  "result": {
    "blockTrace": {
      "blockHash": "08cba31b585b97feb68357fe1b6ca11de2335c346b41abfcbfb6a2667fbfd696",
      "events": [
        {
          "data": {
            "stringValues": {
              "ext_id": "d515",
              "key": "f0c365c3cf59d671eb72da0e7a4113c49f1f0515f462cdcf84e0f1d6045dfcbb",
              "method": "Put",
              "value": "Some(003b9e8b7b010000)",
              "value_encoded": "01003b9e8b7b010000"
            }
          },
          "parentId": null,
          "target": "state"
        },
        {
          "data": {
            "stringValues": {
              "ext_id": "d515",
              "key": "f0c365c3cf59d671eb72da0e7a4113c4bbd108c4899964f707fdaffb82636065",
              "method": "Put",
              "value": "Some(01)",
              "value_encoded": "0101"
            }
          },
          "parentId": null,
          "target": "state"
        },
        {
          "data": {
            "stringValues": {
              "ext_id": "d515",
              "key": "f0c365c3cf59d671eb72da0e7a4113c4bbd108c4899964f707fdaffb82636065",
              "method": "Put",
              "value": "None",
              "value_encoded": "00"
            }
          },
          "parentId": null,
          "target": "state"
        }
      ],
      "methods": "Put",
      "parentHash": "8f193718adebc9de5bab580d0ecc0ac0cc66abb730e487e1fd3cd65ca8855349",
      "spans": [],
      "storageKeys": "f0c365c3cf59d671eb72da0e7a4113c4",
      "tracingTargets": "state"
    }
  },
  "id": 1
}

@emostov emostov self-requested a review August 28, 2021 21:03
Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me

@emostov emostov requested a review from dvdplm August 29, 2021 09:28
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm, modulo a few questions and suggestions.

Comment on lines 267 to 269
/// If an empty string is specified no events will be filtered out. If anything other than
/// an empty string is specified, events will be filtered by method (so non-method events will
/// **not** show up).
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the caller specifies both storage_keys and methods? Is there a priority? Or Invalid Params error (this would be my preference)? Either way it'd be good to document the behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question: what is the expected format of these methods? Is it the Rust name or some other convention (JS?)? A short example here would be valuable to newcomers I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if the caller specifies both storage_keys and methods? Is there a priority?

I prefer the priority of filter conditions to be determined according to the order of declaration of rpc parameters. (Currently, storage_keys is filtered first, then methods)

what is the expected format of these methods?

These methods are just ordinary string format, you could find these methods by rg "method = " primitives/state-machine/src/ext.rs

A short example here would be valuable to newcomers I think.

Yes, I will add a simple example into doc

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the priority of filter conditions to be determined according to the order of declaration of rpc parameters. (Currently, storage_keys is filtered first, then methods)

Fair. Can you document that behaviour please?

client/tracing/src/block/mod.rs Outdated Show resolved Hide resolved
@@ -35,6 +35,9 @@ pub struct BlockTrace {
/// Storage key targets used to filter out events that do not have one of the storage keys.
/// Empty string means do not filter out any events.
pub storage_keys: String,
/// Method targets used to filter out events that do not have one of the event method.
/// Empty string means do not filter out any events.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this empty string business is going to come back and bite us one day. Ideally we'd have an Option<String> here, but I understand why you went with the same we had before.

Signed-off-by: koushiro <[email protected]>
Signed-off-by: koushiro <[email protected]>
Signed-off-by: koushiro <[email protected]>
Signed-off-by: koushiro <[email protected]>
@@ -258,6 +258,15 @@ pub trait StateApi<Hash> {
/// http://localhost:9933/
/// ```
///
/// - Get tracing events with all `storage_keys` and method ('Put')
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@dvdplm dvdplm added A8-mergeoncegreen C1-low PR touches the given topic and has a low impact on builders. labels Aug 30, 2021
@emostov emostov added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Aug 30, 2021
@emostov
Copy link
Contributor

emostov commented Aug 30, 2021

bot merge

@ghost
Copy link

ghost commented Aug 30, 2021

Trying merge.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants