Add transaction signing #7
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7 +/- ##
==========================================
+ Coverage 82.03% 82.51% +0.48%
==========================================
Files 17 20 +3
Lines 757 858 +101
==========================================
+ Hits 621 708 +87
- Misses 136 150 +14
Continue to review full report at Codecov.
|
Sources/Transaction.swift
Outdated
| public var gasLimit: BigInt | ||
| public var recipient: Address? | ||
| public var gasLimit: UInt64 | ||
| public var recipient: Address |
There was a problem hiding this comment.
can we change recipient => to for consitency?
There was a problem hiding this comment.
I believe it could be optional
There was a problem hiding this comment.
I based it on the geth implementation, I think it's more important to be consistent with that: https://github.com/ethereum/go-ethereum/blob/ba1030b6b84f810c04a82221a1b1c0a3dbf499a8/core/types/transaction.go
There was a problem hiding this comment.
Oh I see, I was comparing to JS implementation
| /// - Parameters: | ||
| /// - chainID: chain identifier, defaults to `1` | ||
| /// - hashSigner: function to use for signing the hash | ||
| public mutating func sign(chainID: Int = 1, hashSigner: (Data) throws -> Data) rethrows { |
There was a problem hiding this comment.
Honestly, I would ditch HomesteadSigner at all. everyone use EIP155 for this stuff.
There was a problem hiding this comment.
Again, this is based on geth. I don't think it's harmful to have.
| precondition(signature.count == 65, "Wrong size for signature") | ||
| let r = BigInt(sign: .plus, magnitude: BigUInt(Data(signature[..<32]))) | ||
| let s = BigInt(sign: .plus, magnitude: BigUInt(Data(signature[32..<64]))) | ||
| let v = BigInt(sign: .plus, magnitude: BigUInt(Data(bytes: [signature[64] + 27]))) |
There was a problem hiding this comment.
I remember there was something where we wanted to make 27 customizable? can't remember
Sources/Transaction.swift
Outdated
| @@ -10,13 +10,40 @@ import BigInt | |||
| public struct Transaction { | |||
| public var accountNonce: UInt64 | |||
There was a problem hiding this comment.
can we call it nonce for consistency?
There was a problem hiding this comment.
Again, based on the geth implementation.
There was a problem hiding this comment.
I guess the JSON names do match what you suggest, I'll change it.
Sources/Transaction.swift
Outdated
| public var s = BigInt() | ||
|
|
||
| /// Creates a `Transaction` with default values and the given recipient address. | ||
| public init(recipient: Address) { |
There was a problem hiding this comment.
did you consider leaving initializer for all the values for safety? to avoid any issues, in that case creator would be aware of all the values to pass in, otherwise transaction won't succeed if everything is 0
No description provided.