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

Fix state not using "account_starting_nonce" #1830

Merged
merged 8 commits into from
Aug 4, 2016
Merged

Fix state not using "account_starting_nonce" #1830

merged 8 commits into from
Aug 4, 2016

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Aug 3, 2016

No description provided.

@NikVolf NikVolf added the A0-pleasereview 🤓 Pull request needs code review. label Aug 3, 2016
@NikVolf
Copy link
Contributor Author

NikVolf commented Aug 3, 2016

#1825

@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage decreased (-0.01%) to 86.572% when pulling 9a243f3 on fix-1825 into deceb5f on master.


let secret = Secret::from_str("8a283037bb19c4fed7b1c569e40c7dcff366165eb869110a1b11532963eb9cb2").unwrap();
let tester = EthTester::from_spec_provider(|| Spec::load(POSITIVE_NONCE_SPEC));
let address = tester.accounts.insert_account(secret, "").unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

why inserting it at all? the original fail case was when the account didn't exist, wasn't it?

Copy link
Contributor Author

@NikVolf NikVolf Aug 3, 2016

Choose a reason for hiding this comment

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

@gavofyork
i guess it is when there were no transaction for existing address also?

Copy link
Collaborator

Choose a reason for hiding this comment

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

But it doesn't matter if it's a local account or no, you could test 0x0 adddress and it would be fine.

@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage decreased (-6.6%) to 80.01% when pulling 564812e on fix-1825 into deceb5f on master.

@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage decreased (-6.6%) to 79.998% when pulling 564812e on fix-1825 into deceb5f on master.

@tomusdrw
Copy link
Collaborator

tomusdrw commented Aug 4, 2016

Sorry, about the JSON, I wasn't aware that there are other inline specs in this file, I was rather thinking of putting the files to ethcore/res/ethereum. Maybe separating them as .json files include_str!() would also be a good idea?
But I'm also ok with it now, those are just suggestions :)

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 4, 2016
@NikVolf
Copy link
Contributor Author

NikVolf commented Aug 4, 2016

@tomusdrw
this is out of scope of this one-lined fix :)

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Aug 4, 2016
@arkpar
Copy link
Collaborator

arkpar commented Aug 4, 2016

test is failing

@arkpar arkpar added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 4, 2016
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Aug 4, 2016
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 4, 2016
@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-6.7%) to 79.917% when pulling a401fea on fix-1825 into deceb5f on master.

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Aug 4, 2016
@NikVolf
Copy link
Contributor Author

NikVolf commented Aug 4, 2016

so i changed null_morden.json to make it have zero starting nonce because many test depend on it

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-6.7%) to 79.907% when pulling 7a0bab8 on fix-1825 into deceb5f on master.

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-0.09%) to 86.491% when pulling 05903b9 on fix-1825 into deceb5f on master.

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 4, 2016
@gavofyork gavofyork merged commit aa59aa4 into master Aug 4, 2016
@gavofyork gavofyork deleted the fix-1825 branch August 4, 2016 16:17
arkpar pushed a commit that referenced this pull request Aug 5, 2016
* failng test

* use account_starting_nonce instead of zero

* simplier test

* jsons are getting closer

* incorrect test client and incorrect tests fix

* null_morden is using 0x0 starting nonce

* replaced json with the correct one

* superwhatever line
gavofyork pushed a commit that referenced this pull request Aug 5, 2016
* Fix up the VM trace.

* Fix test.

* Fix state not using "account_starting_nonce" (#1830)

* failng test

* use account_starting_nonce instead of zero

* simplier test

* jsons are getting closer

* incorrect test client and incorrect tests fix

* null_morden is using 0x0 starting nonce

* replaced json with the correct one

* superwhatever line

* Bump json-ipc-server again (#1839)

* bump ipc version

* bumping once more

* v1.2.4

* Fixed reported max height and transaction propagation (#1852)

* Fixed max height and transaction propagation

* Fixed tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants