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(libwaku): support string and int64 for timestamps #3205

Merged
merged 3 commits into from
Dec 10, 2024
Merged

Conversation

richard-ramos
Copy link
Member

The Protocol Buffers JSON Specification for 64 bit numbers, recommends serializing numbers as strings to avoid loss of precision when working with 64-bit integers in JavaScript environments. This causes issues as some json serializers like Go's respect the specification when serializing a protobuffer into Json.
To avoid this issue, in this PR will allow accepting both integers and numeric strings for those cases in which this spec is not followed

Copy link

github-actions bot commented Dec 9, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3205

Built from c459f4e

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

LGTM!
Maybe we could generalize this approach for easy reuse.

Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@richard-ramos
Copy link
Member Author

@NagyZoltanPeter: Maybe we could generalize this approach for easy reuse.

Would something like this work?

import std/[json, strutils]
import results
import ../../../waku/waku_core/time

proc getProtoTimestamp*[T](node: JsonNode, key: string): Opt[T] =
  let (value, ok) =
    if node.hasKey(key):
      if node[key].kind == JString:
        (parseInt(node[key].getStr()), true)
      else:
        (node[key].getBiggestInt(), true)

  if not (T is Timestamp or T is int or T is int64):
    raise newException(ValueError, "Unsupported type")

  return
    if not ok:
      return T.none()
    else:
      when T is Timestamp:
        some(Timestamp(value))
      else:
        some(value)

@NagyZoltanPeter
Copy link
Contributor

@NagyZoltanPeter: Maybe we could generalize this approach for easy reuse.

Would something like this work?

import std/[json, strutils]
import results
import ../../../waku/waku_core/time

proc getProtoTimestamp*[T](node: JsonNode, key: string): Opt[T] =
  let (value, ok) =
    if node.hasKey(key):
      if node[key].kind == JString:
        (parseInt(node[key].getStr()), true)
      else:
        (node[key].getBiggestInt(), true)

  if not (T is Timestamp or T is int or T is int64):
    raise newException(ValueError, "Unsupported type")

  return
    if not ok:
      return T.none()
    else:
      when T is Timestamp:
        some(Timestamp(value))
      else:
        some(value)

Yeah, kind of like that!
Is it only work for Timestamp or can it be named more generic?

@richard-ramos
Copy link
Member Author

@NagyZoltanPeter check the latest commit!
I made it even more generic by returning an int64 instead. I had not realized that Timestamp was an alias for int64

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks so much for it! 🙌

import results
import ../waku/waku_core/time

proc getProtoInt64*(node: JsonNode, key: string): Option[int64] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should return a Result instead so that we can better control any possible error

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to a result now. Please do check latest commit and let me know your thoughts!

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter 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! :-)

@richard-ramos richard-ramos merged commit 2022f54 into master Dec 10, 2024
9 of 11 checks passed
@richard-ramos richard-ramos deleted the proto-str branch December 10, 2024 17:52
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.

4 participants