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

Configuration map of block reward contract addresses #10875

Merged
merged 4 commits into from
Aug 22, 2019
Merged

Configuration map of block reward contract addresses #10875

merged 4 commits into from
Aug 22, 2019

Conversation

vkomenda
Copy link
Contributor

The original issue fixed by this PR's changeset: poanetwork#144.

@parity-cla-bot
Copy link

It looks like @vkomenda hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@jam10o-new jam10o-new added the A0-pleasereview 🤓 Pull request needs code review. label Jul 11, 2019
@vkomenda
Copy link
Contributor Author

[clabot:check]

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@jam10o-new jam10o-new added F8-enhancement 🎊 An additional feature request. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. labels Jul 11, 2019
@vkomenda vkomenda force-pushed the poanetwork-block-reward-transition-map branch 2 times, most recently from a0dcdb8 to 774e998 Compare July 19, 2019 12:02
@afck
Copy link
Contributor

afck commented Jul 22, 2019

Looks like that test is also failing on master.

@ordian
Copy link
Collaborator

ordian commented Jul 22, 2019

please revert submodules update (ethcore/res/ethereum/tests and ethcore/res/wasm-tests)

@ordian ordian added this to the 2.7 milestone Jul 22, 2019
@afck afck force-pushed the poanetwork-block-reward-transition-map branch 2 times, most recently from 811d07e to d24a914 Compare July 25, 2019 10:31
@vkomenda vkomenda force-pushed the poanetwork-block-reward-transition-map branch from d24a914 to 07ee922 Compare August 5, 2019 08:14
@vkomenda
Copy link
Contributor Author

vkomenda commented Aug 5, 2019

Please review.

dvdplm
dvdplm suggested changes Aug 15, 2019
View reviewed changes
Copy link
Collaborator

@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.

Code lgtm, and the feature seems useful to me.

A few things:

  1. a few tests to prove the overriding/merging logic of block_reward_contract_transition with block_reward_contract_transitions so we don't accidentally mess this up in the future
  2. docs with a recommendation to use block_reward_contract_transitions over the singular version? It would also be fantastic to have some docs hinting on the use case for multiple block reward transitions (or maybe it's just me being thick).

And thanks for the contribution!

@vkomenda
Copy link
Contributor Author

@dvdplm, thanks for your feedback! I've added tests and docs. Please check if you are happy with them.

@vkomenda vkomenda force-pushed the poanetwork-block-reward-transition-map branch from 917353f to 9fd2e52 Compare August 20, 2019 15:02
andogro
andogro reviewed Aug 20, 2019
View reviewed changes
json/src/spec/authority_round.rs
/// Block reward contract address (setting the block reward contract overrides the static block
/// reward definition). This option allows to add a single block reward contract address and is
/// compatible with the multiple address option `block_reward_contract_transitions` below.
/// Block reward contract address which overrides the `block_reward` setting. This option allows
Copy link

Choose a reason for hiding this comment

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

This option allows to add
This provides the option to add

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'm a bit mixed up. Can you clarify? The sentence loses its meaning after "and" if we rewrite it this way. Originally: this option allows and this option is compatible. With the change: this provides an option and this is compatible. In the latter case I'm not sure what "this" is referring to.

Copy link

@andogro andogro Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Just attempting to make a complete sentence here rather than change the meaning- maybe would be better to say "this option allows for a single block reward contract..." or "this option allows adding" or another variation that completes the sentence and preserves the meaning?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think @andogro is saying that the wording "…allows to add…" is grammatically a bit off and that it'd improve the text if you reworded it with "…provides the option to add…" (or "set") here and on line 67 above. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would the issue be fixed by changing "...allows to add..." into "...allows one to add..."?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure that works too.

@dvdplm dvdplm added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 21, 2019
@dvdplm dvdplm merged commit efb390e into openethereum:master Aug 22, 2019
@vkomenda vkomenda deleted the poanetwork-block-reward-transition-map branch August 22, 2019 09:14
dvdplm added a commit that referenced this pull request Aug 22, 2019
@dvdplm
Merge branch 'master' into dp/chore/extract-clique
792e84e 
* master:
  Configuration map of block reward contract addresses (#10875)
dvdplm added a commit that referenced this pull request Aug 22, 2019
@dvdplm
Merge branch 'dp/chore/extract-clique' into dp/chore/extract-spec-fro…
70a5f87 
…m-ethcore

* dp/chore/extract-clique:
  Configuration map of block reward contract addresses (#10875)
  Update ethcore/src/snapshot/consensus/mod.rs
  Add a 2/3 quorum option to Authority Round. (#10909)
  Missing import
  Rename supports_warp to snapshot_mode
  Introduce Snapshotting enum to distinguish the type of snapshots a chain uses
  Add an EngineType enum to tighten up Engine.name()
  signers is already a ref
  Update ethcore/engines/clique/src/lib.rs
  Update ethcore/engines/ethash/Cargo.toml
  Update ethcore/engines/basic-authority/Cargo.toml
  Update ethcore/block-reward/Cargo.toml
dvdplm added a commit that referenced this pull request Aug 23, 2019
@dvdplm
Merge branch 'dp/chore/extract-spec-from-ethcore' into dp/chore/sort-…
8a8e90a 
…out-ClientIoMessage

* dp/chore/extract-spec-from-ethcore:
  double semi
  Extract engines to own crates (#10966)
  Fix import
  missing import
  Configuration map of block reward contract addresses (#10875)
  Update ethcore/src/snapshot/consensus/mod.rs
  Add a 2/3 quorum option to Authority Round. (#10909)
  Missing import
  Rename supports_warp to snapshot_mode
  Introduce Snapshotting enum to distinguish the type of snapshots a chain uses
  Add an EngineType enum to tighten up Engine.name()
  signers is already a ref
  Update ethcore/engines/clique/src/lib.rs
  Update ethcore/engines/ethash/Cargo.toml
  Update ethcore/engines/basic-authority/Cargo.toml
  Update ethcore/block-reward/Cargo.toml
dvdplm added a commit that referenced this pull request Aug 25, 2019
@dvdplm
Merge branch 'dp/chore/sort-out-ClientIoMessage' into dp/feature/eip-…
55e74fe 
…1344-add-ChainID-opcode

* dp/chore/sort-out-ClientIoMessage:
  Extract spec to own crate (#10978)
  EIP 2028: transaction gas lowered from 68 to 16 (#10987)
  Fix merge problem
  double semi
  Extract engines to own crates (#10966)
  Fix import
  missing import
  Configuration map of block reward contract addresses (#10875)
  Update ethcore/src/snapshot/consensus/mod.rs
  Add a 2/3 quorum option to Authority Round. (#10909)
  Missing import
  Rename supports_warp to snapshot_mode
  Introduce Snapshotting enum to distinguish the type of snapshots a chain uses
  Add an EngineType enum to tighten up Engine.name()
  signers is already a ref
  Update ethcore/engines/clique/src/lib.rs
  Update ethcore/engines/ethash/Cargo.toml
  Update ethcore/engines/basic-authority/Cargo.toml
  Update ethcore/block-reward/Cargo.toml
@varasev varasev mentioned this pull request Jan 4, 2020
Open
@LukaszRozmej LukaszRozmej mentioned this pull request Feb 7, 2020
Closed
@rakita rakita mentioned this pull request Dec 3, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@andogro andogro andogro left review comments

@dvdplm dvdplm dvdplm approved these changes

@seunlanlege seunlanlege seunlanlege approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. F8-enhancement 🎊 An additional feature request.
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
8 participants
@vkomenda @parity-cla-bot @afck @ordian @dvdplm @andogro @seunlanlege @jam10o-new