Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New freeze option to deactivate block, tx freeze / Fix block options passing #941

Merged
merged 2 commits into from
Nov 6, 2020

Conversation

holgerd77
Copy link
Member

Fixes #940

@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #941 into master will decrease coverage by 0.03%.
The diff coverage is 81.81%.

Impacted file tree graph

Flag Coverage Δ
block 73.08% <75.00%> (-0.02%) ⬇️
blockchain 75.34% <ø> (ø)
common 91.79% <ø> (-0.25%) ⬇️
ethash 82.08% <ø> (ø)
tx 88.54% <100.00%> (+0.17%) ⬆️
vm 87.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@jochem-brouwer
Copy link
Member

I was just working on some extra tests here to verify that these opts are passed and then realized that we also need this freeze in BlockHeader, will add here.

block: block/header: add option carry-over test
tx: add option carry-over test
Copy link
Contributor

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

Yes, this looks like a good solution to me! Tests are solid 💯

@@ -43,6 +43,17 @@ export interface BlockOptions {
* difficulty takes precedence over a provided static `difficulty` value.
*/
calcDifficultyFromHeader?: BlockHeader
/**
* A block object by default gets frozen along initialization. This gives you
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* A block object by default gets frozen along initialization. This gives you
A block object by default gets frozen during initialization. This gives you

Just a tiny idiomatic change here...

This comment block is really helpful...makes the rationale for freezing very clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will leave this out since I don't want to dismiss the approval, hope that's ok.

@@ -70,7 +70,7 @@ export class Block {
uncleHeaders.push(BlockHeader.fromValuesArray(uncleHeaderData, opts))
}

return new Block(header, transactions, uncleHeaders)
return new Block(header, transactions, uncleHeaders, opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@holgerd77 holgerd77 merged commit 7371fdf into master Nov 6, 2020
@holgerd77 holgerd77 deleted the block-tx-freeze-optionality branch November 6, 2020 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Object.freeze(this) in a constructor restricts class extension
3 participants