Upgrade to nim-json-rpc v0.4.2 and chronos v4
#64
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
json-rpc now requires nim-json-serialization to convert types to/from json. Use the nim-json-serialization signatures to call the json serialization lib from nim-codex (should be moved to its own lib)
Was removed in json-rpc
Use {.async: (raises: [...].} where needed
Annotate provider with {.push raises:[].}
Format signatures
- signer procs raise SignerError, provider procs raise ProviderError - WalletError now inherits from SignerError - move wallet module under signers - create jsonrpo moudle under signers - bump nim-json-rpc for null-handling fixes - All jsonrpc provider tests passing, still need to fix others
- removes async: raises from getAddress and signTransaction because derived JsonRpcSigner methods were not being used when dynamically dispatched. Once `raises` was removed from the async annotation, the dynamic dispatch worked again. This is only the case for getAddress and signTransaction. - add gcsafe annotation to wallet.provider so that it matches the base method
EstimateGasError is now a ProviderError (it is a SignerError, and SignerError is a ProviderError), so EstimateGasErrors were not being caught
next step is to: 1. change back any ethers var names that were changed for serialization purposes, eg `from` and `type` 2. move the json util to its own lib
Fixes issue where getAddress and sendTransaction could not be found for MockSigner in tests. The problem was that the async: raises update had not been applied to the MockSigner.
There are too many exceptions to catch individually, including chronos raising CatchableError exceptions in await expansion. There are also many other errors captured inside of the new proc with CatchableError. Instead of making it more complicated and harder to read, I think sticking with excepting CatchableError inside of convertError is a sensible solution
Allows aliasing of de/serialized fields, so revert changes of sender to `from` and transactionType to `type`
This was referenced Feb 8, 2024
Closed
markspanbroek
approved these changes
Feb 8, 2024
| template `or`(a: JsonNode, b: typed): JsonNode = | ||
| if a.isNil: b else: a | ||
|
|
||
| func init*(subscriptions: JsonRpcSubscriptions) = |
There was a problem hiding this comment.
Why not do this in JsonRpcSubscriptions.new()?
There was a problem hiding this comment.
The main purpose was to allow new to create an instance of JsonRpcSubscriptions without initializing services, such as parsing of every message, which may be unnecessary or which can lead to race conditions, when the caller simply needed an object instance.
I am not opposed to putting this in new if you feel strongly about it.
There was a problem hiding this comment.
That makes sense. Perhaps we could call it start then, instead of init?
- add comments to hashes shim - remove .catch from callback condition - derive SignerError from EthersError instead of ProviderError. This allows Providers and Signers to be separate, as Ledger does it, to isolate functionality. Some signer functions now raise both ProviderError and SignerError - Update reverts to check for SignerError - Update ERC-20 method comment
markspanbroek
approved these changes
Feb 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
nim-json-rpc#head, in particular has seen some large breaking changes:setMethodHandlerwhich is how ethers was processing subscription updatescallreturnsJsonStringinstead ofJsonNodenim-json-serializationexclusively instead of%andfromJsonThis PR addresses those breaking changes and upgrades to chronos v4, which supports proper async exception tracking. Ethers now has proper exception tracking support.
Adding
{.push raises:[].}as part of the exception tracking support causesTableswithJsonNodekeys to raise anException(due to a call toJsonNode.hash). The underlying error was a missing forward declaration forhashof a particular type. The workaround for this was to create a shim that adds the missing forward declaration and also adds{.raises: [].}.nim-json-rpcserialization replaced thejsonmarshalmodule with forced serialization vianim-json-serialization. Asnim-json-serializationis not a preferred serializer, json serialization was replaced withnim-serde.