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

Tx: Throw when hash() is called on unsigned legacy transaction #1894

Merged
merged 3 commits into from
May 31, 2022

Conversation

ScottyPoi
Copy link
Contributor

#1890
Part of #1717

  1. Edit hash() in legacyTransaction.ts to throw an error if called on an unsigned transaction to align with Block, Tx: Caching for hash method #1445
  2. Edit test in legacy.spec.ts to expect tx.hash() to throw
  3. Edit tests in inputValue.spec.ts to expect tx.hash() to throw

@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #1894 (e9714a0) into develop (961a5c5) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 85.28% <ø> (ø)
blockchain 83.13% <ø> (ø)
client 77.75% <ø> (+0.08%) ⬆️
common 95.57% <ø> (ø)
devp2p 82.96% <ø> (ø)
ethash 90.81% <ø> (ø)
statemanager 84.24% <ø> (ø)
trie 80.68% <ø> (ø)
tx 92.13% <100.00%> (+0.04%) ⬆️
util 87.29% <ø> (ø)
vm 81.08% <ø> (ø)

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

@ScottyPoi
Copy link
Contributor Author

Not sure if the changes made to tests inputValue.spec.ts were the right move, or if they should be deleted or changed. With the changes to tx.hash(), those tests now fail. I changed them to expect that Error, but that failure is tested in legacy.spec.ts, so doing it again in this test is redundant. Do we like we're losing something by deleting the two tests in inputValue.spec.ts?

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Nice, from my side this can get an approval, really clean "catches" of the respective areas to adopt and/or delete the code, I would say the way the tests are adopted are also ok.

Will put this as "Blocked" for now so that we can first merge in #1892 before anything else on develop.

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

LGTM! @holgerd77 I think we can un-block this now, right?

@holgerd77 holgerd77 force-pushed the tx-legacy-unsigned-throw branch from 2621ccf to e9714a0 Compare May 31, 2022 09:25
@holgerd77
Copy link
Member

@jochem-brouwer yes. 🙂

Rebased this via UI after local test rebase.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

@holgerd77 holgerd77 merged commit 7765b7e into develop May 31, 2022
@holgerd77 holgerd77 deleted the tx-legacy-unsigned-throw branch May 31, 2022 10:01
g11tech pushed a commit that referenced this pull request Jun 2, 2022
* tx: edit legacyTransaction to throw when hash() called on unsigned tx

* tx: adapt legacy.spec test to throw on unsigned tx.hash()

* tx: edit tests which call hash() on unsigned tx
g11tech pushed a commit that referenced this pull request Jun 2, 2022
* tx: edit legacyTransaction to throw when hash() called on unsigned tx

* tx: adapt legacy.spec test to throw on unsigned tx.hash()

* tx: edit tests which call hash() on unsigned tx
g11tech pushed a commit that referenced this pull request Jun 2, 2022
* tx: edit legacyTransaction to throw when hash() called on unsigned tx

* tx: adapt legacy.spec test to throw on unsigned tx.hash()

* tx: edit tests which call hash() on unsigned tx
g11tech pushed a commit that referenced this pull request Jun 3, 2022
* tx: edit legacyTransaction to throw when hash() called on unsigned tx

* tx: adapt legacy.spec test to throw on unsigned tx.hash()

* tx: edit tests which call hash() on unsigned tx
holgerd77 pushed a commit that referenced this pull request Jun 8, 2022
* tx: edit legacyTransaction to throw when hash() called on unsigned tx

* tx: adapt legacy.spec test to throw on unsigned tx.hash()

* tx: edit tests which call hash() on unsigned tx
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.

3 participants