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

EIPs 155, 160, 161 #2976

Merged
merged 29 commits into from
Nov 3, 2016
Merged

EIPs 155, 160, 161 #2976

merged 29 commits into from
Nov 3, 2016

Conversation

gavofyork
Copy link
Contributor

@gavofyork gavofyork commented Oct 29, 2016

Components:

@gavofyork gavofyork added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Oct 29, 2016
@gavofyork gavofyork added the M4-core ⛓ Core client code / Rust. label Oct 29, 2016
@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 30, 2016
@@ -72,8 +72,8 @@ pub struct Transaction {

impl Transaction {
/// Append object with a without signature into RLP stream
pub fn rlp_append_unsigned_transaction(&self, s: &mut RlpStream) {
s.begin_list(6);
pub fn rlp_append_unsigned_transaction(&self, s: &mut RlpStream, network_id: Option<u8>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be made private so that it is not misused for RLP serialization

@@ -295,8 +307,16 @@ impl SignedTransaction {
}
}

/// 0 is `v` is 27, 1 if 28, and 4 otherwise.
pub fn standard_v(&self) -> u8 { match self.v { 27 => 0, 28 => 1, _ => 4 } }
/// 0 if `v` is 27, 1 if 28 or 4 if invalid.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment needs updating

@arkpar arkpar 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 Oct 30, 2016
@gavofyork gavofyork 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 Oct 30, 2016
@gavofyork gavofyork closed this Oct 31, 2016
@gavofyork gavofyork reopened this Oct 31, 2016
@rphmeier
Copy link
Contributor

All these spec file keys are gonna be a bit hellish for users to migrate their private chains.

Global,
Local(u8),
Either(u8),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why's this commented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

needs removing.

@@ -45,10 +50,20 @@ impl Substate {
/// Merge secondary substate `s` into self, accruing each element correspondingly.
pub fn accrue(&mut self, s: Substate) {
self.suicides.extend(s.suicides.into_iter());
self.garbage.extend(s.garbage.into_iter());
Copy link
Contributor

Choose a reason for hiding this comment

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

is it guaranteed that the union of two substates won't contain the same address in two sets?

Copy link
Contributor

Choose a reason for hiding this comment

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

(in expected usage)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, but then it also doesn't matter.

stream.out().sha3()
}

/// Signs the transaction as coming from `sender`.
pub fn sign(self, secret: &Secret) -> SignedTransaction {
let sig = ::ethkey::sign(secret, &self.hash())
pub fn sign(self, secret: &Secret, network_id: Option<u8>) -> SignedTransaction {
Copy link
Contributor

Choose a reason for hiding this comment

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

a convenience function sign_standard = sign(secret, None) would be nice.

Copy link
Contributor Author

@gavofyork gavofyork Oct 31, 2016

Choose a reason for hiding this comment

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

sign_standard would be a misnomer since not providing a network ID is not a "standard" - could be sign_without_network_id, but then i think sign(..., None) says mostly the same without the verbosity and additional fn.

@gavofyork
Copy link
Contributor Author

gavofyork commented Oct 31, 2016

re: private chains, yeah - it's not our fault, though. @keorn has a script to help with this.

ChainEra::TransitionTest => ethereum::new_transition_test().engine,
};

for (name, test) in tests.into_iter() {
/* if name != "ZeroValue_SUICIDE" {
continue;
}*/
Copy link
Contributor

Choose a reason for hiding this comment

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

commented code

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 3, 2016
@gavofyork gavofyork merged commit d3de475 into master Nov 3, 2016
@gavofyork gavofyork deleted the next-hardfork branch November 3, 2016 21:22
@arkpar arkpar mentioned this pull request Nov 3, 2016
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. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants