This repository has been archived by the owner on Apr 6, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 237
Update official transaction tests #131
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
93911c6
Update official transaction tests to run with latest version of ether…
danjm 5ada62d
Fix testRunner to ensure that all tests run and fail correctly
danjm d9c00e8
Run one assertion per test and properly handle txs which are expected…
danjm 249672e
Allow running of a single test according to an exact filename match.
danjm e7b1a57
Refactor transactionRunner for compatability with use of latest ether…
danjm 26b53bf
Ensure transaction runner handles all forks, valid and invalid tests …
danjm b39d17c
ethereumjs-tx support for spurious dragon, as well latest versions of…
danjm e85d975
Support legacy chainId setting alongside new setting of chainId throu…
danjm c15edfb
Fix lint and fake.js tests in response to transaction test and ethere…
danjm 4d75731
Clarify application of EIP155 rules in the hash() method
danjm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
'use strict' | ||
const ethUtil = require('ethereumjs-util') | ||
const Common = require('ethereumjs-common') | ||
const Common = require('ethereumjs-common').default | ||
const BN = ethUtil.BN | ||
|
||
// secp256k1n/2 | ||
|
@@ -151,7 +151,11 @@ class Transaction { | |
if (chainId < 0) chainId = 0 | ||
|
||
// set chainId | ||
this._chainId = chainId || data.chainId || 0 | ||
if (opts.chain || opts.common) { | ||
this._chainId = this._common.chainId() | ||
} else { | ||
this._chainId = chainId || data.chainId || 0 | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -170,16 +174,20 @@ class Transaction { | |
hash (includeSignature) { | ||
if (includeSignature === undefined) includeSignature = true | ||
|
||
// EIP155 spec: | ||
// when computing the hash of a transaction for purposes of signing or recovering, | ||
// instead of hashing only the first six elements (ie. nonce, gasprice, startgas, to, value, data), | ||
// hash nine elements, with v replaced by CHAIN_ID, r = 0 and s = 0 | ||
|
||
let items | ||
if (includeSignature) { | ||
items = this.raw | ||
} else { | ||
if (this._chainId > 0) { | ||
// EIP155 spec: | ||
// If block.number >= 2,675,000 and v = CHAIN_ID * 2 + 35 or v = CHAIN_ID * 2 + 36, then when computing | ||
// the hash of a transaction for purposes of signing or recovering, instead of hashing only the first six | ||
// elements (i.e. nonce, gasprice, startgas, to, value, data), hash nine elements, with v replaced by | ||
// CHAIN_ID, r = 0 and s = 0. | ||
|
||
const onEIP155BlockOrLater = this._common.gteHardfork('spuriousDragon') | ||
const v = ethUtil.bufferToInt(this.v) | ||
const vAndChainIdMeetEIP155Conditions = v === this._chainId * 2 + 35 || v === this._chainId * 2 + 36 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice |
||
if (vAndChainIdMeetEIP155Conditions && onEIP155BlockOrLater) { | ||
const raw = this.raw.slice() | ||
this.v = this._chainId | ||
this.r = 0 | ||
|
@@ -239,11 +247,9 @@ class Transaction { | |
} | ||
|
||
try { | ||
let v = ethUtil.bufferToInt(this.v) | ||
if (this._chainId > 0) { | ||
v -= this._chainId * 2 + 8 | ||
} | ||
this._senderPubKey = ethUtil.ecrecover(msgHash, v, this.r, this.s) | ||
const v = ethUtil.bufferToInt(this.v) | ||
const useChainIdWhileRecoveringPubKey = v >= this._chainId * 2 + 35 && this._common.gteHardfork('spuriousDragon') | ||
this._senderPubKey = ethUtil.ecrecover(msgHash, v, this.r, this.s, useChainIdWhileRecoveringPubKey && this._chainId) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. |
||
} catch (e) { | ||
return false | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,8 +20,8 @@ | |
"author": "mjbecze <[email protected]>", | ||
"license": "MPL-2.0", | ||
"dependencies": { | ||
"ethereumjs-common": "^0.6.1", | ||
"ethereumjs-util": "^5.0.0" | ||
"ethereumjs-common": "^1.0.0", | ||
"ethereumjs-util": "^6.0.0" | ||
}, | ||
"devDependencies": { | ||
"async": "^2.0.0", | ||
|
@@ -33,7 +33,7 @@ | |
"contributor": "^0.1.25", | ||
"coveralls": "^2.11.4", | ||
"documentation": "^8.0.0", | ||
"ethereumjs-testing": "0.0.1", | ||
"ethereumjs-testing": "git+https://github.com/ethereumjs/ethereumjs-testing.git#v1.2.5", | ||
"istanbul": "^0.4.1", | ||
"karma": "^1.1.1", | ||
"karma-browserify": "^5.1.0", | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,64 +1,68 @@ | ||
const Tx = require('../index.js') | ||
const tape = require('tape') | ||
const ethUtil = require('ethereumjs-util') | ||
const Common = require('ethereumjs-common') | ||
const argv = require('minimist')(process.argv.slice(2)) | ||
const testing = require('ethereumjs-testing') | ||
|
||
var txTests = testing.getTests('transaction', argv) | ||
const forkNames = [ | ||
'Byzantium', | ||
'Constantinople', | ||
'EIP150', | ||
'EIP158', | ||
'Frontier', | ||
'Homestead' | ||
] | ||
|
||
const bufferToHex = ethUtil.bufferToHex | ||
const addHexPrefix = ethUtil.addHexPrefix | ||
const stripHexPrefix = ethUtil.stripHexPrefix | ||
const setLength = ethUtil.setLength | ||
|
||
function addPad (v) { | ||
if (v.length % 2 === 1) { | ||
v = '0' + v | ||
} | ||
return v | ||
const forkNameMap = { | ||
Byzantium: 'byzantium', | ||
Constantinople: 'constantinople', | ||
EIP150: 'tangerineWhistle', | ||
EIP158: 'spuriousDragon', | ||
Frontier: 'chainstart', | ||
Homestead: 'homestead' | ||
} | ||
|
||
function normalizeZero (v) { | ||
if (!v || v === '0x') { | ||
return '0x00' | ||
} else { | ||
return v | ||
} | ||
} | ||
tape('TransactionTests', (t) => { | ||
const fileFilterRegex = argv.file ? new RegExp(argv.file + '[^\\w]') : undefined | ||
|
||
testing.runTests(function (testData, sst, cb) { | ||
var tTx = testData.transaction | ||
testing.getTests('TransactionTests', (filename, testName, testData) => { | ||
t.test(testName, (st) => { | ||
const rawTx = ethUtil.toBuffer(testData.rlp) | ||
|
||
try { | ||
var rawTx = ethUtil.toBuffer(testData.rlp) | ||
var tx = new Tx(rawTx, { | ||
hardfork: testData.blockNumber >= 1150000 ? 'homestead' : 'chainstart' | ||
}) | ||
} catch (e) { | ||
sst.equal(undefined, tTx, 'should not have any fields ') | ||
cb() | ||
return | ||
} | ||
let tx | ||
forkNames.forEach(forkName => { | ||
const forkTestData = testData[forkName] | ||
const shouldBeInvalid = Object.keys(forkTestData).length === 0 | ||
try { | ||
tx = new Tx(rawTx, { | ||
hardfork: forkNameMap[forkName], | ||
chain: 1 | ||
}) | ||
|
||
if (tTx && tx.validate()) { | ||
try { | ||
sst.equal(tx._common instanceof Common, true, '_common class attribute') | ||
sst.equal(bufferToHex(tx.data), addHexPrefix(addPad(stripHexPrefix(tTx.data))), 'data') | ||
sst.equal(normalizeZero(bufferToHex(tx.gasLimit)), tTx.gasLimit, 'gasLimit') | ||
sst.equal(normalizeZero(bufferToHex(tx.gasPrice)), tTx.gasPrice, 'gasPrice') | ||
sst.equal(normalizeZero(bufferToHex(tx.nonce)), tTx.nonce, 'nonce') | ||
sst.equal(normalizeZero(bufferToHex(setLength(tx.r, 32))), normalizeZero(bufferToHex(setLength(tTx.r, 32))), 'r') | ||
sst.equal(normalizeZero(bufferToHex(tx.s)), normalizeZero(bufferToHex(tTx.s)), 's') | ||
sst.equal(normalizeZero(bufferToHex(tx.v)), normalizeZero(bufferToHex(tTx.v)), 'v') | ||
sst.equal(bufferToHex(tx.to), addHexPrefix(tTx.to), 'to') | ||
sst.equal(normalizeZero(bufferToHex(tx.value)), tTx.value, 'value') | ||
sst.equal(normalizeZero(bufferToHex(tx.getSenderAddress())), addHexPrefix(testData.sender), "sender's address") | ||
} catch (e) { | ||
sst.fail(e) | ||
} | ||
} else { | ||
sst.equal(undefined, tTx, 'no tx params in test') | ||
} | ||
cb() | ||
}, txTests, tape) | ||
const sender = tx.getSenderAddress().toString('hex') | ||
const hash = tx.hash().toString('hex') | ||
|
||
const validationErrors = tx.validate(true) | ||
const transactionIsValid = validationErrors.length === 0 | ||
const hashAndSenderAreCorrect = forkTestData && (sender === forkTestData.sender && hash === forkTestData.hash) | ||
|
||
if (shouldBeInvalid) { | ||
st.assert(!transactionIsValid, `Transaction should be invalid on ${forkName}`) | ||
} else { | ||
st.assert(hashAndSenderAreCorrect && transactionIsValid, `Transaction should be valid on ${forkName}`) | ||
} | ||
} catch (e) { | ||
if (shouldBeInvalid) { | ||
st.assert(shouldBeInvalid, `Transaction should be invalid on ${forkName}`) | ||
} else { | ||
st.fail(`Transaction should be valid on ${forkName}`) | ||
} | ||
} | ||
}) | ||
st.end() | ||
}) | ||
}, fileFilterRegex) | ||
.then(() => { | ||
t.end() | ||
}) | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good, clean and nice and easy to read test setup! 👍 |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The discussion on here is pretty useful - can we add an inline comment to summarize (or paste the link to comment)? This would help future devs to gather context quickly.