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.

Sign up for GitHub

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

Jump to bottom

JSON: bugs fixes, add unit tests. #141

Merged
merged 18 commits into from
Jul 14, 2021
Merged

JSON: bugs fixes, add unit tests. #141

merged 18 commits into from
Jul 14, 2021

Conversation

sept-en
Copy link
Contributor

@sept-en sept-en commented Jun 12, 2021

No description provided.

@sept-en sept-en added the C-enhancement Category: New feature or request label Jun 12, 2021
sept-en
sept-en commented Jun 12, 2021
View reviewed changes
src/json.rs Outdated Show resolved Hide resolved
@sept-en sept-en changed the title JSON: add unit tests. JSON: bugs fixes, add unit tests. Jun 12, 2021
@mrLSD
Copy link
Member

mrLSD commented Jul 1, 2021

@sept-en what current status of PR?

@joshuajbouw
Copy link
Contributor

@sept-en This is getting stale, whats the status?

@sept-en
Copy link
Contributor Author

sept-en commented Jul 7, 2021

@mrLSD @joshuajbouw this was set as a draft because of dependency on #137. Now it needs to be finished without it as the mentioned PR won't be there in the nearest future. The current PR is a low priority though I will try to resolve it this week.
Thanks for reminding me!

@sept-en sept-en added the tests label Jul 7, 2021
@mrLSD mrLSD self-assigned this Jul 7, 2021
@mrLSD mrLSD marked this pull request as ready for review July 8, 2021 22:26
@mrLSD mrLSD requested a review from artob as a code owner July 8, 2021 22:26
@mrLSD mrLSD requested review from joshuajbouw and birchmd July 8, 2021 22:27
@mrLSD
Copy link
Member

mrLSD commented Jul 8, 2021

@sept-en I completed that PR. I can't request review from you. So just put your feedback.

joshuajbouw
joshuajbouw suggested changes Jul 9, 2021
View reviewed changes
src/json.rs Outdated Show resolved Hide resolved
src/json.rs Show resolved Hide resolved
src/json.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
birchmd
birchmd requested changes Jul 9, 2021
View reviewed changes
Copy link
Member

@birchmd birchmd left a comment

Choose a reason for hiding this comment

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

Thank you for adding lots of new tests! I greatly appreciate it ❤️

src/json.rs Outdated Show resolved Hide resolved
src/json.rs Outdated Show resolved Hide resolved
src/json.rs Outdated Show resolved Hide resolved
src/json.rs Show resolved Hide resolved
src/json.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/json.rs Outdated Show resolved Hide resolved
src/json.rs Show resolved Hide resolved
@joshuajbouw
Copy link
Contributor

In the future @sept-en please link other PRs and say that it is dependent on another else I wouldn't know without talking about it. Please fill out a description too, regardless of how self-explanatory.

@mrLSD
Copy link
Member

mrLSD commented Jul 9, 2021

@birchmd we should decide do we really need to implement new functionality for u64, u128. rjson parser takes the initial number only as f64.

@birchmd
Copy link
Member

birchmd commented Jul 9, 2021

rjson parser takes the initial number only as f64.

That is unfortunate; going through f64 has a possibility of some precision issue I think. We could test this by passing u64::MAX. If there is a precision problem with this (i.e. the value after parsing is not equal to the input value due to rounding in floating point), then maybe we need to consider forking rjson, or contributing upstream, or just writing our own parser to suit our needs.

None of those options is too much work I don't think, but probably would be outside the scope of this PR. We could for example, file an issue for unresolved items and merge this since it is an improvement even if it is not perfect.

@joshuajbouw
Copy link
Contributor

@mrLSD Why not use Serde with the JSON plugin?

@mrLSD
Copy link
Member

mrLSD commented Jul 12, 2021

@mrLSD Why not use Serde with the JSON plugin?

It'll be the ideal solution. But AFAIR it increases contract size.

@birchmd
Copy link
Member

birchmd commented Jul 12, 2021

Why not use Serde with the JSON plugin?

I think we were doing that previously and someone suggested we go for something more minimal because serde is very heavy-weight for our use case.

We could always measure what the impact is on code size (we should eliminate the dependency on secp256k1 first though because we already know the vast majority of the current contract size comes from that).

@joshuajbouw
Copy link
Contributor

Why not use Serde with the JSON plugin?

I think we were doing that previously and someone suggested we go for something more minimal because serde is very heavy-weight for our use case.

We could always measure what the impact is on code size (we should eliminate the dependency on secp256k1 first though because we already know the vast majority of the current contract size comes from that).

I see, good to know.

birchmd
birchmd reviewed Jul 12, 2021
View reviewed changes
src/json.rs Outdated Show resolved Hide resolved
src/json.rs Outdated Show resolved Hide resolved
src/json.rs Outdated Show resolved Hide resolved
@mrLSD mrLSD mentioned this pull request Jul 13, 2021
Closed
joshuajbouw
joshuajbouw suggested changes Jul 13, 2021
View reviewed changes
Copy link
Contributor

@joshuajbouw joshuajbouw left a comment

Choose a reason for hiding this comment

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

Just small ask here.

src/json.rs Outdated Show resolved Hide resolved
src/json.rs Outdated Show resolved Hide resolved
@mrLSD mrLSD mentioned this pull request Jul 13, 2021
Closed
@joshuajbouw joshuajbouw self-requested a review July 13, 2021 12:46
@joshuajbouw joshuajbouw merged commit 3d8e3fd into develop Jul 14, 2021
@joshuajbouw joshuajbouw deleted the json-unit-tests-and-fixes branch July 14, 2021 09:14
artob added a commit that referenced this pull request Jul 30, 2021
@artob @mrLSD
@joshuajbouw @sept-en
Release 1.5.0. (#207)
65a1d11 
* JSON: fix bugs and add unit tests. (#141)
* Add storage layout debug support for `EvmErc20.sol`. (#178)
* ERC-20: forbid using invalid NEP-141 AccountID for mapping. (#179)
* Add EIP-2930 support. (#181, #182)
* Migrate all workflows to self-hosted runners. (#185)
* Speed up the workflow using build caching. (#189)
* Use the new math API host functions. (#190)
* Fix `clippy::enum_variant_names` warning. (#192)
* Add different networks to the Makefile. (#193)
* Update the network status in the README. (#194)
* Remove the toolchain installation step in workflows. (#195)
* Run all tests for all networks in CI. (#196)
* Optimize for performance instead of code size. (#197)
* Parallelize the test suites. (#198)
* Add build-caching to the testing workflow. (#201)
* Refactor tests to use Signer. (#203)
* Add options to the bench profile. (#204)
* Remove a duplicate test. (#205)
* Add a sanity test for access list handling. (#206)
* Update nearcore to the latest branch.
* Add feature gates to the SDK's new host functions.

Co-authored-by: Ahmed Ali <ahmed@aurora.dev>
Co-authored-by: Dmitry Strokov <dmitry@aurora.dev>
Co-authored-by: Evgeny Ukhanov <evgeny@aurora.dev>
Co-authored-by: Joshua J. Bouw <joshua@aurora.dev>
Co-authored-by: Kirill <kirill@aurora.dev>
Co-authored-by: Michael Birch <michael@aurora.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@birchmd birchmd birchmd approved these changes

@joshuajbouw joshuajbouw joshuajbouw approved these changes

@mrLSD mrLSD mrLSD approved these changes

@artob artob Awaiting requested review from artob

Assignees

@mrLSD mrLSD

Labels
C-enhancement Category: New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sept-en @mrLSD @joshuajbouw @birchmd