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

Remove overloaded UInt256.fromJson #74

Merged
merged 4 commits into from
May 21, 2024

Conversation

emizzle
Copy link
Contributor

@emizzle emizzle commented May 17, 2024

Rely instead on UInt256.fromJson from nim-serde to avoid overload symbol collisions.

Bump nim-serde to version that contains a workaround for the following:

When calling createRpcSigs from nim-json-rpc, the readValue mixin is called, which captures all readValue overloads in scope. The issue is that the readValue overload in ethers calls a generic fromJson, and only the fromJson generic overloads defined in ethers are in scope. However, implementing applications, eg nim-codex defined other fromJson generic overloads that are not in scope at the time of the readValue mixin call. The important part of this issue is that both nim-codex and ethers overload the same fromJson (UInt256). The result, is that when nim-codex calls UInt256.fromJson, it will not know about the overload in nim-codex, and instead always call the ethers overload, which is not correct.

This PR removes the ethers UInt256.fromJson overload as a partial solution to the issue. This overload is not necessary as the UInt256.fromJson achieves mostly the same result. The difference is that empty strings were deserialized into 0.u256. This could possibly affect:

  1. BlockTag from being deserialized when there's an empty string: fixed in fix: deserialize BlockTag from empty string #73.
  2. blockNumber (UInt256) from being deserialized when there's an empty string. It appears that deserializing a missing block number from a TransactionReceipt into 0 might have actually caused some issues when waiting on block confirmations, and this change should alleviate any of those issues.

are not all yet in scope at the time of the readValue mixin call (some are, but others are defined in an implementing application, eg nim-codex). This basically forces readValue to use the fromJson from ethers which get muddled up in the readValue mixin scope (), which deserializes an empty string for ?UInt256 into UInt256.none. Previously, .

Rely instead on UInt256.fromJson from nim-serde, which deserializes an empty string for ?UInt256 into UInt256.none. Previously, empty strings were deserialized into 0.u256. BlockNumber was using this deserialization, and it appears that deserializing a missing block number from a TransactionReceipt into 0 might actually cause some issues when waiting on block confirmations.
@emizzle emizzle requested review from markspanbroek, gmega and AuHau May 17, 2024 02:49
@gmega
Copy link
Member

gmega commented May 17, 2024

Awesome, one small observation:

The issue is that the readValue overload in ethers calls a generic fromJson, and only the fromJson generic overloads defined in ethers are in scope.

While you are right that this would also be an issue, in our case the serde calls are actually in scope as well (conversions.nim exports serde). The problem is that the conversion for UInt256 is less specific in serde as it uses T: typedesc[StUint or StInt] whereas the one from ethers is specific for UInt256 so the compiler picks that instead when baking the dispatched call inside of the conversion for Option[UInt256].

Copy link
Member

@gmega gmega left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@markspanbroek markspanbroek left a comment

Choose a reason for hiding this comment

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

I like it. It removes code 😁

Please address the comment, then I think it's good to go!

ethers.nimble Outdated
requires "serde >= 0.1.1 & < 0.2.0"
requires "serde#e889b76dda0636421484dcda1535aa0911345244"
Copy link
Member

Choose a reason for hiding this comment

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

I don't want ethers to be pinned to a specific git hash of serde. Can't you make a release of serde instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch. This was meant to be changed when the serde PR was merged and a release cut. Updated.

@emizzle emizzle merged commit 958d7b4 into main May 21, 2024
4 checks passed
@emizzle emizzle deleted the fix/remove-uint256-fromjson-overload branch May 21, 2024 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants