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

fix jellyfish-json path based bignumber remapping #128

Merged
merged 3 commits into from
Apr 16, 2021

Conversation

fuxingloh
Copy link
Contributor

What kind of PR is this?:

/kind fix

What this PR does / why we need it:

A balancing act between BigNumber parsing and effective usage. I redesigned it to use path based mapping. It will map all within a path to the specific precision. This allow flexibility json parsing and an easy to use interface.

For the bellow setup, only h and d.f.f1 will be number, while the request get remapped to BigNumber.

const losslessObj = parse(`
  {
    "a": 1.2,
    "b": [1.2, 1.2],
    "c": { "c1": 1.2, "c2": [1.2] },
    "d": {
      "e": {
        "e1": 1.2,
        "e2": [1.2, 1.2],
        "e3": { "e3a": 1.2, "e3b": [1.2] }
      },
      "f": {
        "g": 1.2,
        "f1": 1.2
      }
    },
    "h": 1.2
  }
  `)
  const remapped = remap(losslessObj, {
    a: 'bignumber',
    b: 'bignumber',
    c: 'bignumber',
    d: {
      e: 'bignumber',
      f: {
        g: 'bignumber'
      }
    }
  })

I also changed JSON-RPC 1.0 to handle precision on result only.

Which issue(s) does this PR fixes?:

Fixes #104

@netlify
Copy link

netlify bot commented Apr 16, 2021

Deploy preview for jellyfish-defi ready!

Built with commit bb91e5e

https://deploy-preview-128--jellyfish-defi.netlify.app

@fuxingloh fuxingloh requested a review from canonbrother April 16, 2021 10:09
@github-actions
Copy link

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/jellyfish/dist/jellyfish.umd.js 16.63 KB (+0.45% 🔺) 333 ms (+0.45% 🔺) 193 ms (+31.96% 🔺) 526 ms

@codecov
Copy link

codecov bot commented Apr 16, 2021

Codecov Report

Merging #128 (3a2baf2) into main (ac7d29c) will decrease coverage by 0.11%.
The diff coverage is 97.29%.

❗ Current head 3a2baf2 differs from pull request most recent head bb91e5e. Consider uploading reports for the commit bb91e5e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
- Coverage   94.32%   94.21%   -0.12%     
==========================================
  Files          36       36              
  Lines         793      760      -33     
  Branches       92       87       -5     
==========================================
- Hits          748      716      -32     
  Misses         42       42              
+ Partials        3        2       -1     
Impacted Files Coverage Δ
packages/jellyfish-api-core/src/index.ts 100.00% <ø> (ø)
packages/jellyfish-json/src/remap.ts 96.66% <96.42%> (-0.21%) ⬇️
...ish-api-core/__tests__/container_adapter_client.ts 100.00% <100.00%> (ø)
...ages/jellyfish-api-core/src/category/blockchain.ts 100.00% <100.00%> (ø)
packages/jellyfish-api-jsonrpc/src/index.ts 96.77% <100.00%> (ø)
packages/jellyfish-json/src/index.ts 91.66% <100.00%> (+0.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac7d29c...bb91e5e. Read the comment docs.

@fuxingloh fuxingloh marked this pull request as ready for review April 16, 2021 10:20
Comment on lines +88 to +90
const { result, error } = JellyfishJSON.parse(text, {
result: precision
})
Copy link
Contributor

Choose a reason for hiding this comment

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

wow yea this is a great

@fuxingloh fuxingloh enabled auto-merge (squash) April 16, 2021 10:23
@fuxingloh fuxingloh merged commit db5a0b9 into main Apr 16, 2021
@fuxingloh fuxingloh deleted the fuxingloh/jellyfish-json branch April 16, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/fix Fix a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

strict json precision parsing
3 participants