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

Allow default block parameter to be blockHash #10932

Merged
merged 3 commits into from
Aug 6, 2019
Merged

Allow default block parameter to be blockHash #10932

merged 3 commits into from
Aug 6, 2019

Conversation

seunlanlege
Copy link
Member

Adds support for deserializing blockHash and blockNumber as per EIP-1898 to BlockNumber

Closes #10567

@seunlanlege seunlanlege added the M6-rpcapi 📣 RPC API. label Aug 1, 2019
@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@ordian ordian added the A8-looksgood 🦄 Pull request is reviewed well. label Aug 1, 2019
@ordian ordian added this to the 2.7 milestone Aug 1, 2019
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.

LGTM. (requireCanonical is lacking for a full EIP-1898 support)

let s = r#""10""#;
assert!(serde_json::from_str::<BlockNumber>(s).is_err());
fn should_not_deserialize() {
let s = r#"[{}, "10"]"#;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we reject requireCanonical with BlockNumber or any other invalid combinations?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, the code does not reject if used with blockNumber instead it is ignored. Do you think we should reject?

Copy link
Collaborator

@ordian ordian Aug 6, 2019

Choose a reason for hiding this comment

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

I think if we derive Serialize/Deserialize (seems possible with serde attributes) instead of manually implementing it, it would be automatically supported. This can be done in a separate PR though.

@dvdplm dvdplm merged commit 7227985 into master Aug 6, 2019
dvdplm added a commit that referenced this pull request Aug 6, 2019
* master:
  Allow default block parameter to be blockHash (#10932)
  Enable sealing when engine is ready (#10938)
  Fix some warnings and typos. (#10941)
ordian added a commit that referenced this pull request Aug 7, 2019
* master:
  journaldb changes (#10929)
  Allow default block parameter to be blockHash (#10932)
  Enable sealing when engine is ready (#10938)
  Fix some warnings and typos. (#10941)
  Updated security@parity.io key (#10939)
  Change the return type of step_inner function. (#10940)
  get rid of hidden mutability of Spec (#10904)
  simplify BlockReward::reward implementation (#10906)
  Kaspersky AV whitelisting (#10919)
  additional arithmetic EVM opcode benchmarks (#10916)
  [Cargo.lock] cargo update -p crossbeam-epoch (#10921)
  Fixes incorrect comment. (#10913)
  Add file path to disk map write/read warnings (#10911)
@@ -233,6 +233,8 @@ pub trait BlockChainClient : Sync + Send + AccountData + BlockChain + CallContra
.expect("code will return Some if given BlockId::Latest; qed")
}

fn chain(&self) -> Arc<BlockProvider>;
Copy link
Contributor

Choose a reason for hiding this comment

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

That triggered another compiler warning for me:

warning: missing documentation for a trait method
   --> ethcore/src/client/traits.rs:236:2
    |
236 |     fn chain(&self) -> Arc<BlockProvider>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: lint level defined here
   --> ethcore/src/lib.rs:17:9
    |
17  | #![warn(missing_docs, unused_extern_crates)]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, missed this during review. I added docs in another PR so it'll sort itself out soon enough.

ordian added a commit that referenced this pull request Aug 15, 2019
* master:
  [evmbin] fix compilation (#10976)
  Update to latest trie version. (#10972)
  [blooms-db] Fix benchmarks (#10974)
  Fix ethcore/benches build. (#10964)
  tx-pool: accept local tx with higher gas price when pool full (#10901)
  Disable unsyncable expanse chain (#10926)
  Extract Machine from ethcore (#10949)
  removed redundant state_root function from spec, improve spec error types (#10955)
  Add support for Energy Web Foundation's new chains (#10957)
  [evmbin] add more tests to main.rs (#10956)
  Fix compiler warnings in util/io and upgrade to edition 2018 Upgrade mio to latest (#10953)
  unify loading spec && further spec cleanups (#10948)
  refactor: Refactor evmbin CLI (#10742)
  journaldb changes (#10929)
  Allow default block parameter to be blockHash (#10932)
  Enable sealing when engine is ready (#10938)
  Fix some warnings and typos. (#10941)
  Updated security@parity.io key (#10939)
  Change the return type of step_inner function. (#10940)
@niklasad1 niklasad1 deleted the EIP-1898 branch September 13, 2019 13:19
niklasad1 pushed a commit that referenced this pull request Nov 8, 2019
* allow default block parameter to be blockHash

* requireCanonical

* block check takes precedence over canon check
@fabioberger
Copy link

It's been nearly 4 months since this got merged. When is it going to be released? Geth support is already out.

@dvdplm
Copy link
Collaborator

dvdplm commented Nov 25, 2019

Is it not in the latest beta?

@niklasad1
Copy link
Collaborator

niklasad1 commented Nov 25, 2019

@fabioberger @dvdplm It is included in v2.6.5 beta

EDIT: I forgot to add it in the PR description and thus it's missing in the CHANGELOG.

@soc1c soc1c mentioned this pull request Nov 25, 2019
35 tasks
@fabioberger
Copy link

Thanks @niklasad1! 🙏 Any estimates on when it'll make it into the stable releases?

@fabioberger
Copy link

@dvdplm any idea when this will make it into a stable release? I've seen several stable releases go out but none include these changes.

@niklasad1
Copy link
Collaborator

niklasad1 commented Dec 19, 2019

@fabioberger

Sorry late answer, I expect it to be included in stable 2.6.x

@fabioberger
Copy link

Thanks for the reply @niklasad1. Any idea when stable 2.6.x will be released? I've been waiting for ~2 months for this to be released so I rely on it in production.

@niklasad1
Copy link
Collaborator

niklasad1 commented Jan 29, 2020

@fabioberger It will be included stable 2.7, I expect it to be released soon.

EDIT: It is released, https://github.com/paritytech/parity-ethereum/releases/tag/v2.7.0

@riptl
Copy link

riptl commented Oct 2, 2020

Since this is not part of OpenEthereum 3.1 history anymore, I backported this in openethereum/openethereum#70

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. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EIP 1898: allow default block parameter to be blockHash
8 participants