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

Bump nim-serialization and nim-json-serialization #14058

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

jangko
Copy link
Contributor

@jangko jangko commented Mar 20, 2024

What does the PR do

Bump nim-serialization and nim-json-serialization submodule as the latter is super outdated.
This PR also a first step in preparation to bump nim-json-rpc and nim-web3.

Affected areas

Everywhere nim-json-serialization is being used.

@status-im-auto
Copy link
Member

status-im-auto commented Mar 20, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 2e936b8 #1 2024-03-20 10:12:37 ~7 min tests/nim 📄log
✔️ 2e936b8 #1 2024-03-20 10:12:59 ~7 min macos/aarch64 🍎dmg
✔️ 2e936b8 #1 2024-03-20 10:17:18 ~11 min tests/ui 📄log
✔️ 2e936b8 #1 2024-03-20 10:18:12 ~12 min macos/x86_64 🍎dmg
✔️ 2e936b8 #1 2024-03-20 10:23:02 ~17 min linux/x86_64 📦tgz
✔️ 2e936b8 #1 2024-03-20 10:35:42 ~30 min windows/x86_64 💿exe

@jangko jangko marked this pull request as ready for review March 20, 2024 10:42
Copy link
Contributor

@igor-sirotin igor-sirotin left a comment

Choose a reason for hiding this comment

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

@jangko, how big are the changes? Do we need to update the usage of these libraries, or is it safe?

@jangko
Copy link
Contributor Author

jangko commented Jun 18, 2024

Changes in nim-json-serialization are big, almost a complete rewrite of the lexer and parser. And it improves the safety and add protection against bad inputs. Also add a lot of switch to accomodate common practices of abusing json format while at the same time user can choose to use strictly standard json.

The same goes to the nim-json-rpc and nim-web3 library. It improves a lot of type safety and json conversion safety. The new json rpc also support batch call. We try to keep as much backward compatibility in nim-json-rpc. But in nim-web3 we cannot avoid breaking changes, because of super outdated implementation against the latest spec.

@arnetheduck
Copy link
Member

From what I've seen, status desktop is mostly using std/json which is not affected by nim-json-serialization - that said, std/json has lots of performance and safety issues, so wherever we can, we tend to prefer nim-json-serialization - it's a slightly different model though since n-j-s works with nim objects while std/json works with "stringly typing".

All in all, these updates are the result of security and conformance reviews of web3/json-ser and as soon as you start using it, you'll have fewer runtime bugs - using these libraries can let you do more things conveniently in nim without having to pay the go interop costs.

@arnetheduck arnetheduck merged commit 6d96745 into status-im:master Jun 18, 2024
8 checks passed
@osmaczko
Copy link
Contributor

Hey @jangko, thanks for upgrade.

Unfortunately, we're currently experiencing some issues with serialization. It appears that there was a breaking change in how enum values are serialized in nim-serialization. For example, the TokenCriteriaDto::type is now being serialized to a string (enum name) instead of a number.

"tokenCriteria": [
    {
        "contract_addresses": {
            "11155420": "0x0000000000000000000000000000000000000000",
            "421614": "0x0000000000000000000000000000000000000000",
            "11155111": "0x0000000000000000000000000000000000000000"
        },
        "type": "ERC20", <--- HERE
        "symbol": "ETH",
        "name": "",
        "amount": "1000000000000000000",
        "decimals": 18,
        "tokenIds": [],
        "ens_pattern": "",
        "amountInWei": "1000000000000000000"
    }
],

Could you please give me some hints on how we should approach this?

@jangko
Copy link
Contributor Author

jangko commented Jun 19, 2024

A quick fix is to add this before you call the Json.writeFile or Json.encode

proc writeValue*(w: var JsonWriter, v: TokenType)  {.gcsafe, raises: [IOError].} =
    w.writeValue($v.int)

@osmaczko
Copy link
Contributor

A quick fix is to add this before you call the Json.writeFile or Json.encode

proc writeValue*(w: var JsonWriter, v: TokenType)  {.gcsafe, raises: [IOError].} =
    w.writeValue($v.int)

I would need to do that for every enum type we serialize, right? That's a bit annoying. Is there a more generic way?

osmaczko added a commit that referenced this pull request Jun 19, 2024
@jangko
Copy link
Contributor Author

jangko commented Jun 19, 2024

That will need a fix in nim-json-serialization or add a new feature

@arnetheduck
Copy link
Member

That will need a fix in nim-json-serialization or add a new feature

I think we may want to go back to the old behavior here - not sure why it was changed cc @jangko

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.

6 participants