Skip to content

mini-rollup of several issues #498

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Mar 23, 2022
Merged

mini-rollup of several issues #498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e9b53c0
Restore failing #440 PR
mloit Mar 21, 2022
30711bb
PR #440 bugfix, replace the fallthrough's with the desired action, as…
mloit Mar 21, 2022
b6000e9
fix TransactionOptions decoding of callOnBlock using incorrect key of…
mloit Mar 21, 2022
7fd2043
bugfix: if let makes no sense if the input won't produce an optional
mloit Mar 23, 2022
4774e99
fix typo in function name
mloit Mar 23, 2022
11517aa
fix unsafe forced unwrap in transaction signers
mloit Mar 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions Sources/web3swift/EthereumABI/ABIDecoding.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ extension ABIDecoder {
var toReturn = [AnyObject]()
var consumed: UInt64 = 0
for i in 0 ..< types.count {
let (v, c) = decodeSignleType(type: types[i], data: data, pointer: consumed)
let (v, c) = decodeSingleType(type: types[i], data: data, pointer: consumed)
guard let valueUnwrapped = v, let consumedUnwrapped = c else {return nil}
toReturn.append(valueUnwrapped)
consumed = consumed + consumedUnwrapped
Expand All @@ -32,7 +32,7 @@ extension ABIDecoder {
return toReturn
}

public static func decodeSignleType(type: ABI.Element.ParameterType, data: Data, pointer: UInt64 = 0) -> (value: AnyObject?, bytesConsumed: UInt64?) {
public static func decodeSingleType(type: ABI.Element.ParameterType, data: Data, pointer: UInt64 = 0) -> (value: AnyObject?, bytesConsumed: UInt64?) {
let (elData, nextPtr) = followTheData(type: type, data: data, pointer: pointer)
guard let elementItself = elData, let nextElementPointer = nextPtr else {
return (nil, nil)
Expand Down Expand Up @@ -117,7 +117,7 @@ extension ABIDecoder {
var subpointer: UInt64 = 32;
var toReturn = [AnyObject]()
for _ in 0 ..< length {
let (v, c) = decodeSignleType(type: subType, data: elementItself, pointer: subpointer)
let (v, c) = decodeSingleType(type: subType, data: elementItself, pointer: subpointer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it just me, or these lines are equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spelling correction from gn to ng in Single. it's hard to spot

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, now i see

guard let valueUnwrapped = v, let consumedUnwrapped = c else {break}
toReturn.append(valueUnwrapped)
subpointer = subpointer + consumedUnwrapped
Expand All @@ -134,7 +134,7 @@ extension ABIDecoder {
var toReturn = [AnyObject]()
// print("Dynamic array sub element itself: \n" + dataSlice.toHexString())
for _ in 0 ..< length {
let (v, c) = decodeSignleType(type: subType, data: dataSlice, pointer: subpointer)
let (v, c) = decodeSingleType(type: subType, data: dataSlice, pointer: subpointer)
guard let valueUnwrapped = v, let consumedUnwrapped = c else {break}
toReturn.append(valueUnwrapped)
if (subType.isStatic) {
Expand All @@ -152,7 +152,7 @@ extension ABIDecoder {
var toReturn = [AnyObject]()
var consumed:UInt64 = 0
for _ in 0 ..< length {
let (v, c) = decodeSignleType(type: subType, data: elementItself, pointer: consumed)
let (v, c) = decodeSingleType(type: subType, data: elementItself, pointer: consumed)
guard let valueUnwrapped = v, let consumedUnwrapped = c else {return (nil, nil)}
toReturn.append(valueUnwrapped)
consumed = consumed + consumedUnwrapped
Expand All @@ -170,10 +170,28 @@ extension ABIDecoder {
var toReturn = [AnyObject]()
var consumed:UInt64 = 0
for i in 0 ..< subTypes.count {
let (v, c) = decodeSignleType(type: subTypes[i], data: elementItself, pointer: consumed)
let (v, c) = decodeSingleType(type: subTypes[i], data: elementItself, pointer: consumed)
guard let valueUnwrapped = v, let consumedUnwrapped = c else {return (nil, nil)}
toReturn.append(valueUnwrapped)
consumed = consumed + consumedUnwrapped
/*
When decoding a tuple that is not static or an array with a subtype that is not static, the second value in the tuple returned by decodeSignleType is a pointer to the next element, NOT the length of the consumed element. So when decoding such an element, consumed should be set to consumedUnwrapped, NOT incremented by consumedUnwrapped.
*/
switch subTypes[i] {
case .array(type: let subType, length: _):
if !subType.isStatic {
consumed = consumedUnwrapped
} else {
consumed = consumed + consumedUnwrapped
}
case .tuple(types: _):
if !subTypes[i].isStatic {
consumed = consumedUnwrapped
} else {
consumed = consumed + consumedUnwrapped
}
default:
consumed = consumed + consumedUnwrapped
}
}
// print("Tuple element is: \n" + String(describing: toReturn))
if type.isStatic {
Expand Down Expand Up @@ -253,11 +271,11 @@ extension ABIDecoder {
let data = logs[i+1]
let input = indexedInputs[i]
if !input.type.isStatic || input.type.isArray || input.type.memoryUsage != 32 {
let (v, _) = ABIDecoder.decodeSignleType(type: .bytes(length: 32), data: data)
let (v, _) = ABIDecoder.decodeSingleType(type: .bytes(length: 32), data: data)
guard let valueUnwrapped = v else {return nil}
indexedValues.append(valueUnwrapped)
} else {
let (v, _) = ABIDecoder.decodeSignleType(type: input.type, data: data)
let (v, _) = ABIDecoder.decodeSingleType(type: input.type, data: data)
guard let valueUnwrapped = v else {return nil}
indexedValues.append(valueUnwrapped)
}
Expand Down
8 changes: 4 additions & 4 deletions Sources/web3swift/Transaction/TransactionSigner.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public struct Web3Signer {
guard let unmarshalledSignature = SECP256K1.unmarshalSignature(signatureData: serializedSignature) else {
return false
}
let originalPublicKey = SECP256K1.privateToPublic(privateKey: privateKey)
guard let originalPublicKey = SECP256K1.privateToPublic(privateKey: privateKey) else { return false }
var d = BigUInt(0)
if unmarshalledSignature.v >= 0 && unmarshalledSignature.v <= 3 {
d = BigUInt(35)
Expand All @@ -64,7 +64,7 @@ public struct Web3Signer {
transaction.r = BigUInt(Data(unmarshalledSignature.r))
transaction.s = BigUInt(Data(unmarshalledSignature.s))
let recoveredPublicKey = transaction.recoverPublicKey()
if (!(originalPublicKey!.constantTimeComparisonTo(recoveredPublicKey))) {
if !(originalPublicKey.constantTimeComparisonTo(recoveredPublicKey)) {
return false
}
return true
Expand All @@ -89,7 +89,7 @@ public struct Web3Signer {
guard let unmarshalledSignature = SECP256K1.unmarshalSignature(signatureData: serializedSignature) else {
return false
}
let originalPublicKey = SECP256K1.privateToPublic(privateKey: privateKey)
guard let originalPublicKey = SECP256K1.privateToPublic(privateKey: privateKey) else { return false }
transaction.chainID = nil
var d = BigUInt(0)
var a = BigUInt(0)
Expand All @@ -104,7 +104,7 @@ public struct Web3Signer {
transaction.r = BigUInt(Data(unmarshalledSignature.r))
transaction.s = BigUInt(Data(unmarshalledSignature.s))
let recoveredPublicKey = transaction.recoverPublicKey()
if (!(originalPublicKey!.constantTimeComparisonTo(recoveredPublicKey))) {
if !(originalPublicKey.constantTimeComparisonTo(recoveredPublicKey)) {
return false
}
return true
Expand Down
8 changes: 4 additions & 4 deletions Sources/web3swift/Web3/Web3+Structures.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,13 @@ extension TransactionOptions: Decodable {

public init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
if let gasLimit = try decodeHexToBigUInt(container, key: .gas) {
if let gasLimit = try decodeHexToBigUInt(container, key: .gas, allowOptional:true) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

While working with Web3+structures source i'm kind of confused:

As i see this lib took it's struct from web3.js, where some struct fields are optional. But at the same time i've found Eth JSON-RPC specification Transaction scheme where none of that are optional. It looks kind official tho. I've lack of experience in Ethereum itself, so maybe you're could explain to me why there's such ignorance of specification happening even in kind of it's official implementation like web3.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem here is that to code expects to drop down to provide a default if the call to decodeHexToBigUInt returns with nil. decodeHexToBigUInt will throw Web3Error.dataError instead of return nil unless allowOptional is set to true. So this if let is meaningless without the allowOptional: true.

On a larger scope, yes nonce should never be optional, I was not trying to address that issue. That should be handled at a higher level. The input here could come from a number of sources, where nonce may no be known yet. The same goes for the various gas values, as if this is a 1559 transaction those values may not exist. Furthermore, if the transaction is pending and not mined yet, those values may also not be present, as they are not known yet. Issue #469

self.gasLimit = .manual(gasLimit)
} else {
self.gasLimit = .automatic
}

if let gasPrice = try decodeHexToBigUInt(container, key: .gasPrice) {
if let gasPrice = try decodeHexToBigUInt(container, key: .gasPrice, allowOptional:true) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's i see that optionality happening since this struct should work with both EIP-1559 Transactions and legacy transaction where the first one don't have that field already, am i correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as my previous comment

self.gasPrice = .manual(gasPrice)
} else {
self.gasPrice = .automatic
Expand All @@ -85,13 +85,13 @@ extension TransactionOptions: Decodable {
let value = try decodeHexToBigUInt(container, key: .value)
self.value = value

if let nonce = try decodeHexToBigUInt(container, key: .nonce) {
if let nonce = try decodeHexToBigUInt(container, key: .nonce, allowOptional:true) {
self.nonce = .manual(nonce)
} else {
self.nonce = .pending
}

if let callOnBlock = try decodeHexToBigUInt(container, key: .nonce) {
if let callOnBlock = try decodeHexToBigUInt(container, key: .callOnBlock, allowOptional:true) {
self.callOnBlock = .exactBlockNumber(callOnBlock)
} else {
self.callOnBlock = .pending
Expand Down