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

strict json precision parsing #104

Closed
canonbrother opened this issue Apr 9, 2021 · 4 comments · Fixed by #128
Closed

strict json precision parsing #104

canonbrother opened this issue Apr 9, 2021 · 4 comments · Fixed by #128
Assignees
Labels
kind/feature New feature request priority/important-soon Will be important soon triage/accepted Triage has been accepted

Comments

@canonbrother
Copy link
Contributor

canonbrother commented Apr 9, 2021

What would you like to be added:

Its not critical but also a note.
Currently the precision is loose on
while:

parse (obj, type): any
parse({ foo: { bar: 123 }, bar: 123 }, { bar: 'bignumber' }

result:

{ foo: { bar: { BigNumber }}, bar: { BigNumber }}

expected:

{ foo: { bar: 123 }, bar: { BigNumber }}

note:
Two issues will be facing while implement the strict precision

  1. ApiClient.call's resp object structure - { result, error, id }
    Ideally for rpc call is only passing the relevant precision mapping object
// better
getBlock(...args) {
  return await this.client.call(m, [], { number: 'bignumber' })
}

// Not so friendly
getBlock(...args) {
  return await this.client.call(m, [], { result: { number: 'bignumber' }})
}
  1. Nested object will not be able to get parsed
    Currently in order to remove the loose precision is to replace the precision by empty object {}
// remap.ts ln: 108-110
case MappingAction.DEEPLY_UNKNOWN:
  -- remapLosslessObj(losslessObj[k], precision)
  ++ remapLosslessObj(losslessObj[k], {})
  break

this causes the nested object cannot be parsed due to empty precision.

Why is this needed:

  • technically {foo: { bar: 123 }} !== { bar: 123 }
  • for note

But its also convince-able to remain the loose precision

  1. Rare case in real scenario
parse({foo: {bar: 123}, bar: 123}}, {bar: 123})

assume bar is a crypto amount, so why not parsing both??

  1. Its doable for but its required extra struct specification
parse({foo: {bar: 123}, bar: 123}}, { foo: {bar: 'number'}, bar: 'bignumber' })
// foo.bar remains number
// bar is converted to bignumber
@canonbrother canonbrother added the kind/feature New feature request label Apr 9, 2021
@defichain-bot defichain-bot added the needs/triage Waiting for triage to be accepted label Apr 9, 2021
@defichain-bot
Copy link
Member

@canonbrother: Thanks for opening an issue, it is currently awaiting triage.

The triage/accepted label can be added by foundation members by writing /triage accepted in a comment.

Details

I am a bot created to help the DeFiCh developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the DeFiCh/oss-governance-bot repository.

@defichain-bot defichain-bot added the needs/area Needs area label(s) label Apr 9, 2021
@defichain-bot
Copy link
Member

@canonbrother: There are no 'area' labels on this issue. Adding an appropriate label will greatly expedite the process for us. You can add as many area as you see fit. If you are unsure what to do you can ignore this!

You can add area labels by leaving a /area comment.

Details

I am a bot created to help the DeFiCh developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the DeFiCh/oss-governance-bot repository.

@fuxingloh fuxingloh changed the title stict precision strict precision Apr 9, 2021
@fuxingloh fuxingloh added area/jellyfish-json triage/accepted Triage has been accepted labels Apr 9, 2021
@fuxingloh fuxingloh changed the title strict precision strict json precision parsing Apr 9, 2021
@defichain-bot defichain-bot removed needs/triage Waiting for triage to be accepted needs/area Needs area label(s) labels Apr 9, 2021
@fuxingloh fuxingloh added the priority/important-soon Will be important soon label Apr 12, 2021
@fuxingloh
Copy link
Contributor

Thinking this simi simple way to do strict json precision parsing. Instead of "mapping", we could do "path" based.

const object = {
  value: 0,
  values: [
    9,
    9,
  ],
  items: [
    {
      a: 10,
      b: 11,
    },
    {
      a: 20,
      b: 21,
    }
  ],
  deep: {
    c: 0,
    d: 0,
  }
}

// you either define bignumber or it default to number
const pathPrecision = {
  value: 'bignumber', // 0 = bignumber
  values: 'bignumber', // [9,9] = bignumber
  items: {
    a: 'bignumber',  // items: [{a: 10}, {a: 20}] = bignumber
                     // items: [{b: 11}, {b: 21}] = number
  },
  deep: 'bignumber' // {c:0,d:0} get parsed to bignumber
}

@fuxingloh
Copy link
Contributor

https://github.com/DeFiCh/jellyfish/pull/119/checks?check_run_id=2341762608

@fuxingloh a edge case to take note of when implementing strict precision parsing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature request priority/important-soon Will be important soon triage/accepted Triage has been accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants