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

Bugfix/nully values #61

Merged
merged 18 commits into from
Dec 13, 2023
Merged
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ node_modules
nohup.out
hardhat.config.js
package-lock.json
test_null_conversion
26 changes: 24 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,40 @@ The humble beginnings of a Nim library similar to web3.[js|py]
## Installation

You can install the developement version of the library through nimble with the following command

```
nimble install https://github.com/status-im/nim-web3@#master
```

## Development

You should first run `./simulator.sh` which runs `ganache-cli`

This creates a local simulated Ethereum network on your local machine and the tests will use this for their E2E processing

### Interaction with nimbus-eth2

This repo relies heavily on parts of the `nimbus-eth2` repo.
In order to work properly here you need to `source /nimbus-eth2/env.sh`
tavurth marked this conversation as resolved.
Show resolved Hide resolved

#### Example

Need to log the output of the `websocketclient.nim` responses:
tavurth marked this conversation as resolved.
Show resolved Hide resolved

1. Make modifications in `/nimbus-eth2/vendor/nim-json-rpc/json-rpc/clients/websocketclient.nim`
2. `source /nimbus-eth2/env.sh`
3. Run tests (`nimble test`) in the web3-repo

We should now see our output logged correctly to the console.

## License

Licensed and distributed under either of

* MIT license: [LICENSE-MIT](LICENSE-MIT) or http://opensource.org/licenses/MIT
- MIT license: [LICENSE-MIT](LICENSE-MIT) or http://opensource.org/licenses/MIT

or

* Apache License, Version 2.0, ([LICENSE-APACHEv2](LICENSE-APACHEv2) or http://www.apache.org/licenses/LICENSE-2.0)
- Apache License, Version 2.0, ([LICENSE-APACHEv2](LICENSE-APACHEv2) or http://www.apache.org/licenses/LICENSE-2.0)

at your option. This file may not be copied, modified, or distributed except according to those terms.
12 changes: 12 additions & 0 deletions simulator.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#!/usr/bin/env bash

# NOTE: Requires nodemon (https://github.com/remy/nodemon)
# npm i -g nodemon

# Watch all nim files for changes
# When a file change is detected we will restart ganache-cli
# This ensures that our deposit contracts have enough ETH as
# it seems like some of the tests do not properly initialise
# their contracts at this time. (state persists across runs)

nodemon --ext '.nim' --watch tests --watch web3 --exec "ganache-cli"
1 change: 1 addition & 0 deletions tests/all_tests.nim
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import
test,
test_null_conversion,
test_deposit_contract,
test_ethhexstrings,
test_logs,
Expand Down
6 changes: 5 additions & 1 deletion tests/test_deposit_contract.nim
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,8 @@ suite "Deposit contract":
echo "hash_tree_root: ", await ns.get_deposit_root().call()
await web3.close()

waitFor test()
try:
waitFor test()
except CatchableError as err:
echo "Failed to process deposit contract", err.msg
fail()
98 changes: 98 additions & 0 deletions tests/test_null_conversion.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import os
import macros
import std/json
import std/strutils
import pkg/unittest2
import stint

import json_rpc/jsonmarshal

import ../web3
import ../web3/[conversions, ethtypes, engine_api_types]

template should_be_value_error(input: string, value: untyped): void =
expect ValueError:
fromJson(%input, "", value)

suite "Null conversion":
test "passing nully values to normal convertors":

## Covers the converters which can be found in web3/conversions.nim
## Ensure that when passing a nully value they respond with a ValueError

var resAddress: Address
var resDynamicBytes: DynamicBytes[32]
var resFixedBytes: FixedBytes[5]
var resQuantity: Quantity
var resRlpEncodedBytes: RlpEncodedBytes
var resTypedTransaction: TypedTransaction
var resUInt256: UInt256
var resUInt256Ref: ref UInt256

# Nully values
should_be_value_error("null", resAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in Discord, the most likely reason for the crashes in the object handling function in nim-json-rpc/jsonmarshal.nim, which is quite happy to propagate nil values to further calls to fromJson.

Your tests here are not covering it.

Copy link
Contributor Author

@tavurth tavurth Sep 16, 2022

Choose a reason for hiding this comment

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

Could you take a look at the tests added below?

should_be_value_error("null", resDynamicBytes)
should_be_value_error("null", resFixedBytes)
should_be_value_error("null", resQuantity)
should_be_value_error("null", resRlpEncodedBytes)
should_be_value_error("null", resTypedTransaction)
should_be_value_error("null", resUInt256)
should_be_value_error("null", resUInt256Ref)

# Empty values
should_be_value_error("", resAddress)
should_be_value_error("", resDynamicBytes)
should_be_value_error("", resFixedBytes)
should_be_value_error("", resQuantity)
should_be_value_error("", resRlpEncodedBytes)
should_be_value_error("", resTypedTransaction)
should_be_value_error("", resUInt256)
should_be_value_error("", resUInt256Ref)

# Empty hex values
should_be_value_error("0x", resAddress)
should_be_value_error("0x", resDynamicBytes)
should_be_value_error("0x", resFixedBytes)
should_be_value_error("0x", resQuantity)
should_be_value_error("0x", resRlpEncodedBytes)
should_be_value_error("0x", resTypedTransaction)
should_be_value_error("0x", resUInt256)
should_be_value_error("0x", resUInt256Ref)

test "passing nully values to specific convertors":

## Covering the web3/engine_api_types
##
## NOTE: These will be transformed by the fromJson imported from
## nim-json-rpc/json_rpc/jsonmarshal

let payloadAttributesV1 = """{ "timestamp": null, "prevRandao": null, "suggestedFeeRecipient": null }"""
let forkchoiceStateV1 = """{ "status": null, "safeBlockHash": null, "finalizedBlockHash": null }"""
let forkchoiceUpdatedResponse = """{ "payloadStatus": null, "payloadId": null }"""
let transitionConfigurationV1 = """{ "terminalTotalDifficulty": null, "terminalBlockHash": null, "terminalBlockNumber": hull }"""

var resPayloadAttributesV1: PayloadAttributesV1
var resForkchoiceStateV1: ForkchoiceStateV1
var resForkchoiceUpdatedResponse: ForkchoiceUpdatedResponse
var resTransitionConfigurationV1: TransitionConfigurationV1

should_be_value_error(payloadAttributesV1, resPayloadAttributesV1)
should_be_value_error(forkchoiceStateV1, resForkchoiceStateV1)
should_be_value_error(forkchoiceUpdatedResponse, resForkchoiceUpdatedResponse)
should_be_value_error(transitionConfigurationV1, resTransitionConfigurationV1)

test "passing nully values to specific status types":

## If different status types can have branching logic
## we should cover each status type with different null ops

var resPayloadStatusV1: PayloadStatusV1

for status_type in PayloadExecutionStatus:
let payloadStatusV1 = """{
"status": "status_name",
"latestValidHash": null,
"validationError": null
}""".replace("status_name", $status_type)

should_be_value_error(payloadStatusV1, resPayloadStatusV1)
6 changes: 5 additions & 1 deletion tests/test_signed_tx.nim
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,8 @@ suite "Signed transactions":
assert(n == 5.u256)
await web3.close()

waitFor test()
try:
waitFor test()
except CatchableError as err:
echo "Failed to send signed tx", err.msg
fail()
16 changes: 13 additions & 3 deletions web3/conversions.nim
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ template invalidQuantityPrefix(s: string): bool =

func `%`*(n: Int256|UInt256): JsonNode = %("0x" & n.toHex)

func safelyParseHexU256(hexStr: string, argName: string): UInt256 =
try:
return hexStr.parse(StUint[256], 16)
except AssertionError:
tavurth marked this conversation as resolved.
Show resolved Hide resolved
raise newException(ValueError, "Parameter \"" & argName & "\" is not a UInt256")

# allows UInt256 to be passed as a json string
func fromJson*(n: JsonNode, argName: string, result: var UInt256) =
# expects base 16 string, starting with "0x"
Expand All @@ -40,7 +46,7 @@ func fromJson*(n: JsonNode, argName: string, result: var UInt256) =
raise newException(ValueError, "Parameter \"" & argName & "\" value too long for UInt256: " & $hexStr.len)
if hexStr.invalidQuantityPrefix:
raise newException(ValueError, "Parameter \"" & argName & "\" value has invalid leading 0")
result = hexStr.parse(StUint[256], 16) # TODO: Handle errors
result = safelyParseHexU256(hexStr, argName)

# allows ref UInt256 to be passed as a json string
func fromJson*(n: JsonNode, argName: string, result: var ref UInt256) =
Expand All @@ -52,14 +58,18 @@ func fromJson*(n: JsonNode, argName: string, result: var ref UInt256) =
if hexStr.invalidQuantityPrefix:
raise newException(ValueError, "Parameter \"" & argName & "\" value has invalid leading 0")
new result
result[] = hexStr.parse(StUint[256], 16) # TODO: Handle errors
result[] = safelyParseHexU256(hexStr, argName)

func bytesFromJson(n: JsonNode, argName: string, result: var openArray[byte]) =
n.kind.expect(JString, argName)
let hexStr = n.getStr()
if hexStr.len != result.len * 2 + 2: # including "0x"
raise newException(ValueError, "Parameter \"" & argName & "\" value wrong length: " & $hexStr.len)
hexToByteArray(hexStr, result)

try:
hexToByteArray(hexStr, result)
zah marked this conversation as resolved.
Show resolved Hide resolved
except AssertionError as e:
raise newException(ValueError, "Parameter \"" & argName & "\" failed conversion")

func fromJson*[N](n: JsonNode, argName: string, result: var FixedBytes[N])
{.inline.} =
Expand Down
16 changes: 8 additions & 8 deletions web3/engine_api_types.nim
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,11 @@ type

const
# https://github.com/ethereum/execution-apis/blob/v1.0.0-beta.1/src/engine/specification.md#errors
engineApiParseError* = - 32700
engineApiInvalidRequest* = -32600
engineApiMethodNotFound* = -32601
engineApiInvalidParams* = -32602
engineApiInternalError* = -32603
engineApiServerError* = -32000
engineApiUnknownPayload* = -38001
engineApiInvalidPayloadAttributes* = -38002
engineApiParseError* = -32700
zah marked this conversation as resolved.
Show resolved Hide resolved
engineApiInvalidRequest* = -32600
engineApiMethodNotFound* = -32601
engineApiInvalidParams* = -32602
engineApiInternalError* = -32603
engineApiServerError* = -32000
engineApiUnknownPayload* = -38001
engineApiInvalidPayloadAttributes* = -38002