Updates for Chronos V4 #61

gmega · 2023-12-15T14:25:08Z

This PR adds minimal adjustments to ensure that nim-ethers works (i.e. compiles and the test suite passes) with Chronos V4. Changes are minimal enough that it should still work with Chronos 3.x, though that's more of a bonus than a goal.

Two of the more controversial points in this PR are:

the {.cast(raises: []).} in table operations which 1.6.x will otherwise (erroneously) declare as raisers of Exception. I had asked for this in nim-forum with a smaller example and Araq suggested using {.experimental: "strictEffects".} (https://forum.nim-lang.org/t/10749) which indeed fixed my example, but for reasons I can't understand it doesn't work here. The problem seems to not exist with 2.x;
the bump to nim 1.6.16, which I had to do to work around what appeared to be memory getting corrupted with 1.6.14. I did a lot of digging into Chronos and couldn't really find an issue there, so I'm chalking this off as a compiler issue.

dryajov · 2023-12-15T21:11:16Z

ethers/signer.nim

@@ -26,10 +26,12 @@ proc raiseEstimateGasError(
    parent: parent)
  raise e

-method provider*(signer: Signer): Provider {.base, gcsafe.} =
+method provider*(signer: Signer):
+    Provider {.base, gcsafe, raises: [CatchableError].} =


Hmm... I would not blanked raise CatchableError, in fact I would set this to raises: [] and only list those exceptions that are intended to propagate. The whole point of this is preventing spurious exceptions from bubbling up, otherwise there is no point in tracking exceptions at all.

In fact I would use the push: raises [] pragma in all the files and only override this per method on an as needed basis.

Yeah, I mean, as I was working through this, I was faced with two choices:

do the minimal amount of changes so that the current implementation works with Chronos V4;

actually track the entire exception chains and expose the exceptions that all method implementations may throw, which is what you're proposing.

(2) seems like a good idea, but it is actually sort of tricky. Without some feeling as to how the API has to be designed, I could easily replace CatchableError with EthersError pretty much everywhere, and it would compile without much gain in exception semantics. The actual narrow types for the exceptions that would make sense to expose depends on what we expect clients to want to try to recover from, so that's why I ended up going with the (1) route.

I'll give it a stab anyhow, let's see what comes out of it.

Yeah, I understand, this gets messy pretty quickly, but I still think that push pragma with raises: [] and an explicit EthersError where required is a much better than the bare CatchableError. Also, I suspect that a lot of issues are stemming from the fact that callbacks aren't annotated with raises: [].

Its actually not that bad. Here's a compiling commit with {.push raises:[].}: 7973ce8

dryajov · 2023-12-17T23:59:25Z

ethers/providers/jsonrpc/subscriptions.nim

@@ -14,6 +14,14 @@ type
    callbacks: Table[JsonNode, SubscriptionCallback]
  SubscriptionCallback = proc(id, arguments: JsonNode) {.gcsafe, raises:[].}

+# FIXME Nim 1.6.XX seems to have issues tracking exception effects and will see


Actually, this is probably the wrong way of doing it, as you noted in the forum post, annotating the callback type with a raises: [] fixes the issue, this is in fact the correct fix. Callbacks should (almost) never allow exceptions to escape.

Also, here is a pointer on how to handle the strict effect changes in a backwards/forwards compatible way in the 1.6.0 release notes - https://github.com/narimiran/Nim/blob/f14925d3bc778bcb7eef4eb9085f08d65f4281ce/changelogs/changelog_1_6_0.md#strict-effects, should be helpful.

As @dryajov and araq mentioned, the correct fix is to use raises:[] in the callback definition instead of mitigateEffectsBug. The unhandled Exception being raised (and the difference between your example in the forum post and the current implementation in ethers) is that the Table uses a JsonNode as a key. As a guess, the Exception could possibly come from use of parseJson in Table's key handling when the key type is JsonNode. Here's a compiling commit that uses string as key and no mitigateEffectsBug. Note that this commit is not necessarily complete or the correct way to completely remove JsonNode as the callback table key; it's to illustrate that the issue indeed lies in the use of JsonNode as a table key: a1eb549

In summary, the solution would be a combination of using raises:[] in the callback definition, and changing the callback table key type.

Note that table accessors such as get or [] will raise an exception, so this should either use the non throwing versions (getOrDefault, etc), or handle the exceptions in the callback itself.

Awesome, so we have two issues that end in the same symptom (gotta love when that happens):

the nim effects issue (https://forum.nim-lang.org/t/10749);

using JsonNode as a table key.

I'm not sure I follow your explanation though @emizzle (and that's probably the problem :-)): is parseJson called during the hashing of JsonNode?

@dryajov yes but in this case we're writing to the table ([]=), this should not raise exceptions.

emizzle

Exception handling is always super tricky!

emizzle · 2023-12-19T19:37:04Z

ethers.nimble

 requires "chronicles >= 0.10.3 & < 0.11.0"
-requires "chronos >= 3.0.0 & < 4.0.0"
+requires "chronos#head" # FIXME: change to >= 4.0.0 when chronos 4 is released


Would it be better to reference the specific chronos commit instead of head?

Yeah... I think both are bad TBH. 😄 The tradeoff I see is:

using a specific commit buys you stability for the nim-ethers build in the short term, but will require you to constantly bump this to keep up with the latest V4 commits;

using head means every time someone pushes you'll be testing against the latest pre-release of V4, and you may be able to catch issues earlier. Can be annoying to someone who just want to change unrelated stuff and all of a sudden finds themselves with a Chronos error unrelated to their change, though.

Either one sort of sucks IMHO, so I'd be fine with either.

emizzle · 2023-12-19T20:49:27Z

ethers/signer.nim

@@ -26,10 +26,12 @@ proc raiseEstimateGasError(
    parent: parent)
  raise e

-method provider*(signer: Signer): Provider {.base, gcsafe.} =
+method provider*(signer: Signer):
+    Provider {.base, gcsafe, raises: [CatchableError].} =


Its actually not that bad. Here's a compiling commit with {.push raises:[].}: 7973ce8

emizzle · 2023-12-19T20:54:15Z

ethers/providers/jsonrpc/subscriptions.nim

@@ -14,6 +14,14 @@ type
    callbacks: Table[JsonNode, SubscriptionCallback]
  SubscriptionCallback = proc(id, arguments: JsonNode) {.gcsafe, raises:[].}

+# FIXME Nim 1.6.XX seems to have issues tracking exception effects and will see


As @dryajov and araq mentioned, the correct fix is to use raises:[] in the callback definition instead of mitigateEffectsBug. The unhandled Exception being raised (and the difference between your example in the forum post and the current implementation in ethers) is that the Table uses a JsonNode as a key. As a guess, the Exception could possibly come from use of parseJson in Table's key handling when the key type is JsonNode. Here's a compiling commit that uses string as key and no mitigateEffectsBug. Note that this commit is not necessarily complete or the correct way to completely remove JsonNode as the callback table key; it's to illustrate that the issue indeed lies in the use of JsonNode as a table key: a1eb549

emizzle · 2023-12-19T21:03:19Z

ethers/providers/jsonrpc/subscriptions.nim

@@ -14,6 +14,14 @@ type
    callbacks: Table[JsonNode, SubscriptionCallback]
  SubscriptionCallback = proc(id, arguments: JsonNode) {.gcsafe, raises:[].}

+# FIXME Nim 1.6.XX seems to have issues tracking exception effects and will see


In summary, the solution would be a combination of using raises:[] in the callback definition, and changing the callback table key type.

emizzle · 2024-02-08T04:47:37Z

Are we ok to close this in favour of #64?

gmega requested a review from markspanbroek December 15, 2023 14:26

gmega added 2 commits December 15, 2023 11:35

initial exception types and effects fixes

ce4d8d0

switch chronos and asynctest to head; bump nim to 1.6.16

8ca2b37

gmega force-pushed the feat/chronos-v4 branch from 522ff48 to 8ca2b37 Compare December 15, 2023 14:35

set correct version for asynctest

cbaf3c7

dryajov reviewed Dec 15, 2023

View reviewed changes

dryajov reviewed Dec 17, 2023

View reviewed changes

emizzle reviewed Dec 19, 2023

View reviewed changes

gmega mentioned this pull request Jan 22, 2024

Update to Chronos V4 codex-storage/nim-codex#673

Closed

markspanbroek mentioned this pull request Feb 5, 2024

Fix decoding of getter functions #63

Merged

markspanbroek closed this Mar 12, 2024

markspanbroek deleted the feat/chronos-v4 branch March 12, 2024 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates for Chronos V4 #61

Updates for Chronos V4 #61

gmega commented Dec 15, 2023 •

edited

Loading

dryajov Dec 15, 2023

dryajov Dec 15, 2023

gmega Dec 15, 2023 •

edited

Loading

dryajov Dec 18, 2023

emizzle Dec 19, 2023

dryajov Dec 17, 2023 •

edited

Loading

dryajov Dec 18, 2023

emizzle Dec 19, 2023

emizzle Dec 19, 2023

dryajov Dec 20, 2023

gmega Jan 5, 2024 •

edited

Loading

emizzle left a comment

emizzle Dec 19, 2023

gmega Jan 5, 2024

emizzle Dec 19, 2023

emizzle Dec 19, 2023

emizzle Dec 19, 2023

emizzle commented Feb 8, 2024 •

edited

Loading

Updates for Chronos V4 #61

Updates for Chronos V4 #61

Conversation

gmega commented Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmega Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmega Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

emizzle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emizzle commented Feb 8, 2024 • edited Loading

gmega commented Dec 15, 2023 •

edited

Loading

gmega Dec 15, 2023 •

edited

Loading

dryajov Dec 17, 2023 •

edited

Loading

gmega Jan 5, 2024 •

edited

Loading

emizzle commented Feb 8, 2024 •

edited

Loading