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

Type annotation for next_key() matching of json filter options #11192

Merged
merged 3 commits into from Oct 23, 2019

Conversation

lamafab
Copy link
Contributor

@lamafab lamafab commented Oct 22, 2019

Fixes #11189. Very small and easy changes.

Example

"params": [
  15,
  {
    "to": {
        "eq": "0xe8b2d01ffa0a15736b2370b6e5064f9702c891b6"
    },
    "gas": {
        "gt": "0x493e0"
    },
  }
]

curl example:

curl --data '{"method":"parity_pendingTransactions","id": 1,"params":[5,{"from":{"eq":"0x5f3dffcf347944d3739b0805c934d86c8621997f"},"to": {"eq":"0xe8b2d01ffa0a15736b2370b6e5064f9702c891b6"}}],"jsonrpc":"2.0"}' -H "Content-Type: application/json" -X POST localhost:8545

EDIT:
PR for documentation https://github.com/paritytech/wiki/pull/345

@parity-cla-bot
Copy link

It looks like @lamafab signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Thanks!

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. M6-rpcapi 📣 RPC API. labels Oct 22, 2019
@ordian ordian added this to the 2.7 milestone Oct 22, 2019
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Nice, thank you

Do you mind adding a test for this:

I used the following earlier:

diff --git a/rpc/src/v1/tests/mocked/parity.rs b/rpc/src/v1/tests/mocked/parity.rs
index 913e181e4..35d43b624 100644
--- a/rpc/src/v1/tests/mocked/parity.rs
+++ b/rpc/src/v1/tests/mocked/parity.rs
@@ -322,6 +322,28 @@ fn rpc_parity_pending_transactions() {
        assert_eq!(io.handle_request_sync(request), Some(response.to_owned()));
 }
 
+#[test]
+fn rpc_parity_pending_transactions_with_data_missing_limit() {
+       let deps = Dependencies::new();
+       let io = deps.default_client();
+
+       let request = r#"{"jsonrpc": "2.0", "method": "parity_pendingTransactions", "params":[{"from": {"eq": "0x5f3dffcf347944d3739b0805c934d86c8621997f"}, "to": { "eq": "0xe8b2d01ffa0a15736b2370b6e5064f9702c891b6"}}], "id":1}"#;
+       let response = r#"{"jsonrpc":"2.0","result":[],"id":1}"#;
+
+       assert_eq!(io.handle_request_sync(request), Some(response.to_owned()));
+}
+
+#[test]
+fn rpc_parity_pending_transactions_with_data() {
+       let deps = Dependencies::new();
+       let io = deps.default_client();
+
+       let request = r#"{"jsonrpc": "2.0", "method": "parity_pendingTransactions", "params":[5, {"from": {"eq": "0x5f3dffcf347944d3739b0805c934d86c8621997f"}, "to": { "eq": "0xe8b2d01ffa0a15736b2370b6e5064f9702c891b6"}}], "id":1}"#;
+       let response = r#"{"jsonrpc":"2.0","result":[],"id":1}"#;
+
+       assert_eq!(io.handle_request_sync(request), Some(response.to_owned()));
+}
+
 #[test]
 fn rpc_parity_encrypt() {
        let deps = Dependencies::new();

@lamafab
Copy link
Contributor Author

lamafab commented Oct 22, 2019

Should the tests just cover JSON validity or actually contain filtering logic? I remember trying to do that but had trouble creating sample test transactions. I was told we'll create those eventually, but never got down to it.

(do note that the filter_options already have simple unit tests for JSON validity, but those do not cover the full RPC call, as shown by this bug).

@niklasad1
Copy link
Collaborator

niklasad1 commented Oct 22, 2019

Should the tests just cover JSON validity or actually contain filtering logic? I remember trying to do that but had trouble creating sample test transactions. I was told we'll create those eventually, but never got down to it.

All right, the tests should ideally cover filtering logic but JSON deserialization is better than nothing. It is up to you. I see it requires some additional mock code to create some blocks with to and from I suppose. I think something like the following would do it:

	use v1::helpers::fake_sign;
	use types::transaction::*;
	use std::str::FromStr;

	let from: Address = Address::from_str("5f3dffcf347944d3739b0805c934d86c8621997f").unwrap();

	let deps = Dependencies::new();
	{
		let miner = deps.miner.clone();
		for i in 0..10 {
			 // maybe add some different fields here
                         // action field will contain receiver address
                         let tx = Transaction::default().fake_sign(from);
			 // it will have the same hash let's create dummy hash
                         miner.pending_transactions.lock().insert([i as u8; 32].into(), tx);
		}
	}
	let io = deps.default_client();

	let request = r#"{"jsonrpc": "2.0", "method": "parity_pendingTransactions", "params":[1, {"from": {"eq": "0x5f3dffcf347944d3739b0805c934d86c8621997f"}}], "id":1}"#;

@lamafab
Copy link
Contributor Author

lamafab commented Oct 23, 2019

@niklasad1 I just updated the PR. This includes some basic JSON deserialization tests for the pending transactions RPC API. There should be logic tests, which I can try to implement when I have some spare time, but I can't right now.

Also, in the README.md I mentioned to pull the git submodules required for running tests.

@@ -151,7 +151,7 @@ To start Parity Ethereum as a regular user using `systemd` init:

## 4. Testing <a id="chapter-004"></a>

You can run tests with the following commands:
Download the required test files: `git submodule update --init --recursive`. You can run tests with the following commands:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does cargo test --all fail without this? I thought it was needed only for cargo test --all --features=json-tests but maybe I'm wrong?

Copy link
Collaborator

@niklasad1 niklasad1 Oct 23, 2019

Choose a reason for hiding this comment

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

Yes, it fails we have this macro which requires the ethereum tests submodule which is used in a couple of the rpc tests

Copy link
Contributor Author

@lamafab lamafab Oct 23, 2019

Choose a reason for hiding this comment

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

I don't know if with --all, but it did fail for me when running cargo test -p parity-json

Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

neat!

@debris debris added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Oct 23, 2019
@debris debris merged commit acf7c48 into openethereum:master Oct 23, 2019
dvdplm added a commit that referenced this pull request Oct 24, 2019
* master:
  Type annotation for next_key() matching of json filter options (#11192)
  Crypto primitives removed from ethkey (#11174)
  Made ecrecover implementation trait public (#11188)
  Remove unused macro_use. (#11191)
  [dependencies]: jsonrpc `14.0.1` (#11183)
  [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179)
  [dependencies] bump rand 0.7 (#11022)
  [ethcore/builtin]: do not panic in blake2pricer on short input (#11180)
  TxPermissions ver 3: gas price & data (#11170)
  [ethash] chainspec validate `ecip1017EraRounds` non-zero (#11123)
  util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175)
  ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
  Aura: Report malice on sibling blocks from the same validator (#11160)
dvdplm added a commit that referenced this pull request Oct 24, 2019
* master:
  Type annotation for next_key() matching of json filter options (#11192)
  Crypto primitives removed from ethkey (#11174)
dvdplm added a commit that referenced this pull request Oct 24, 2019
* master:
  Pause pruning while snapshotting (#11178)
  Type annotation for next_key() matching of json filter options (#11192)
  Crypto primitives removed from ethkey (#11174)
  Made ecrecover implementation trait public (#11188)
  Remove unused macro_use. (#11191)
  [dependencies]: jsonrpc `14.0.1` (#11183)
  [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179)
  [dependencies] bump rand 0.7 (#11022)
  [ethcore/builtin]: do not panic in blake2pricer on short input (#11180)
  TxPermissions ver 3: gas price & data (#11170)
  [ethash] chainspec validate `ecip1017EraRounds` non-zero (#11123)
  util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175)
  ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
  Aura: Report malice on sibling blocks from the same validator (#11160)
This was referenced Nov 5, 2019
niklasad1 pushed a commit that referenced this pull request Nov 5, 2019
* type annotation for next_key matching of json filter options

* rpc tests for pending transactions

* mention git submodules in the readme
dvdplm added a commit that referenced this pull request Nov 6, 2019
* master: (21 commits)
  ropsten #6631425 foundation #8798209 (#11201)
  Update list of bootnodes for xDai chain (#11236)
  ethcore/res: add mordor testnet configuration (#11200)
  [chain specs]: activate `Istanbul` on mainnet (#11228)
  [builtin]: support `multiple prices and activations` in chain spec (#11039)
  Insert explicit warning into the panic hook (#11225)
  Snapshot restoration overhaul (#11219)
  Fix docker centos build (#11226)
  retry on gitlab system failures (#11222)
  Update bootnodes. (#11203)
  Use provided usd-per-eth value if an endpoint is specified (#11209)
  Use a lock instead of atomics for snapshot Progress (#11197)
  [informant]: `MillisecondDuration` -> `as_millis()` (#11211)
  Step duration map configuration parameter ported from the POA Network fork (#10902)
  Upgrade jsonrpc to latest (#11206)
  [export hardcoded sync]: use debug for `H256` (#11204)
  Pause pruning while snapshotting (#11178)
  Type annotation for next_key() matching of json filter options (#11192)
  Crypto primitives removed from ethkey (#11174)
  Made ecrecover implementation trait public (#11188)
  ...
dvdplm pushed a commit that referenced this pull request Nov 7, 2019
* type annotation for next_key matching of json filter options

* rpc tests for pending transactions

* mention git submodules in the readme
s3krit pushed a commit that referenced this pull request Nov 11, 2019
* ropsten #6631425 foundation #8798209 (#11201)
* [stable] builtin, istanbul and mordor testnet backports (#11234)
  * ethcore-builtin (#10850)
  * [builtin]: support `multiple prices and activations` in chain spec (#11039)
  * [chain specs]: activate `Istanbul` on mainnet (#11228)
  * ethcore/res: add mordor testnet configuration (#11200)
* Update list of bootnodes for xDai chain (#11236)
* ethcore: remove `test-helper feat` from build (#11047)
* Secret store: fix Instant::now() related race in net_keep_alive (#11155) (#11159)
* [stable]: backport #10691 and #10683 (#11143)
  * Fix compiler warning (that will become an error) (#10683)
  * Refactor Clique stepping (#10691)
* Add Constantinople eips to the dev (instant_seal) config (#10809)
* Add cargo-remote dir to .gitignore (?)
* Insert explicit warning into the panic hook (#11225)
* Fix docker centos build (#11226)
* Update MIX bootnodes. (#11203)
* Use provided usd-per-eth value if an endpoint is specified (#11209)
* Add new line after writing block to hex file. (#10984)
* Type annotation for next_key() matching of json filter options (#11192) (but no `FilterOption` in 2.5 so…)
* Upgrade jsonrpc to latest (#11206)
* [CI] check evmbin build (#11096)
* Correct EIP-712 encoding (#11092)
* [client]: Fix for incorrectly dropped consensus messages (#11086)
* Fix block detail updating (#11015)
* Switching sccache from local to Redis (#10971)
* Made ecrecover implementation trait public (#11188)
* [dependencies]: jsonrpc `14.0.1` (#11183)
* [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179)
* [ethcore/builtin]: do not panic in blake2pricer on short input (#11180)
* util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175)
* ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
* Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127)
* Cleanup stratum a bit (#11161)
* Upgrade to jsonrpc v14 (#11151)
* SecretStore: expose restore_key_public in HTTP API (#10241)
s3krit pushed a commit that referenced this pull request Nov 11, 2019
* [CI] check evmbin build (#11096)
* Correct EIP-712 encoding (#11092)
* [client]: Fix for incorrectly dropped consensus messages (#11082) (#11086)
* Update hardcoded headers (foundation, classic, kovan, xdai, ewc, ...) (#11053)
* Add cargo-remote dir to .gitignore (?)
* Update light client headers: ropsten 6631425 foundation 8798209 (#11201)
* Update list of bootnodes for xDai chain (#11236)
* ethcore/res: add mordor testnet configuration (#11200)
* [chain specs]: activate Istanbul on mainnet (#11228)
* [builtin]: support multiple prices and activations in chain spec (#11039)
* [receipt]: add sender & receiver to RichReceipts (#11179)
* [ethcore/builtin]: do not panic in blake2pricer on short input (#11180)
* Made ecrecover implementation trait public (#11188)
* Fix docker centos build (#11226)
* Update MIX bootnodes. (#11203)
* Insert explicit warning into the panic hook (#11225)
* Use provided usd-per-eth value if an endpoint is specified (#11209)
* Cleanup stratum a bit (#11161)
* Add Constantinople EIPs to the dev (instant_seal) config (#10809) (already backported)
* util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175)
* ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
* Type annotation for next_key() matching of json filter options (#11192)
* Upgrade jsonrpc to latest (#11206)
* [dependencies]: jsonrpc 14.0.1 (#11183)
* Upgrade to jsonrpc v14 (#11151)
* Switching sccache from local to Redis (#10971)
* Snapshot restoration overhaul (#11219)
* Add new line after writing block to hex file. (#10984)
* Pause pruning while snapshotting (#11178)
* Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127)
* Fix block detail updating (#11015)
* Make InstantSeal Instant again #11186
* Filter out some bad ropsten warp snapshots (#11247)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can no longer filter transactions with RPC API
6 participants