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

Fix TestUpdatedKeyfileContents, TestEIP155SigningVitalik and TestChainId #354

Merged
merged 3 commits into from
May 23, 2018

Conversation

tsuzukit
Copy link
Contributor

For fixing TestUpdatedKeyfileContents, I have cherry-picked c86c126 and c8b5da7 which are already merged to geth 1.7.3.

For fixing TestEIP155SigningVitalik, I needed to change chainId to something other than 1 as it results in V of 37 and Tx is treated as protected transaction in Quorum (https://github.com/jpmorganchase/quorum/blob/master/core/types/transaction.go#L138).
Since http://vitalik.ca/files/eip155_testvec.txt is not currently accessible, I just created some tx and address pair. (tx creation rules are written in comments)

For same reason, I needed to change chainId for TestChainId.

Please review.

@jpmsam
Copy link
Contributor

jpmsam commented Apr 26, 2018

@tsuzukit Thank you for this, we'll be reviewing the changes and EIP155. In the meantime, please complete our CLA

@tsuzukit
Copy link
Contributor Author

@jpmsam
Hi, thank you for the response and review!
I have sent CLA to [email protected].

@nujabes403
Copy link

nujabes403 commented May 2, 2018

Please merge this request quickly.. 'Invalid sender' error is critical for further developing..

@jpmsam
Copy link
Contributor

jpmsam commented May 2, 2018

@nujabes403 we are working on testing it. Please provide your feedback on whether the updates fixed the issue for you as well.

@tsuzukit
Copy link
Contributor Author

tsuzukit commented May 7, 2018

Actually, #350 is the fix to "invalid sender".
This pull request is intended to fix some unit tests.

@jpmsam
Copy link
Contributor

jpmsam commented May 7, 2018

@tsuzukit thanks, we are in the process of testing both.

@xiaods
Copy link

xiaods commented May 22, 2018

the master branch it test failing in macos, please review and merge it asap.

@jpmsam jpmsam merged commit 4a77480 into Consensys:master May 23, 2018
@tsuzukit tsuzukit deleted the feature/fix-some-tests branch May 29, 2018 16:00
Szkered pushed a commit to Szkered/quorum that referenced this pull request Jun 22, 2018
Fix TestUpdatedKeyfileContents, TestEIP155SigningVitalik and TestChainId
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants