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

feat: forgive timestamps provided as ms/μs #13

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Bilb
Copy link
Collaborator

@Bilb Bilb commented Dec 12, 2024

No description provided.

@jagerman
Copy link
Member

I think that this change more properly belongs in the binding code for nodejs, so that what libsession-nodejs would expose is a function to nodejs that takes millisecond input, and produces millisecond output, as those are what native nodejs tools expect, and does the ÷1000 or ×1000 in the function wrapper.

Milliseconds unix timestamps are a non-common concept in general; pretty much every other language defines timestamps in terms of seconds: plain integer seconds (older C apis, and languages that adopted that convention), a seconds/nanosecond pair (newer C APIs, Java), or a floating point seconds value (Python, Swift, and loads of other languages). Older Java used milliseconds, but when they revamped the date time API to fix various limitations in it a decade ago they moved away from it. That's not to say all language should be the same, by any means, but it's the job of the specific language binding code to pave over conceptual differences in what the specific language wants, rather than make the library worry about possible different language conventions.

@mpretty-cyro
Copy link
Collaborator

I think that this change more properly belongs in the binding code for nodejs, so that what libsession-nodejs would expose is a function to nodejs that takes millisecond input, and produces millisecond output, as those are what native nodejs tools expect, and does the ÷1000 or ×1000 in the function wrapper.

This is unfortunately less of a nodejs-specific thing and more an attempt to fix some libSession misuse that happened at some point - there was some invalid conversions on one or more of the platforms which resulted in incorrect timestamps being added into libSession which, unsurprisingly, caused a bunch of bugs

We could look at implementing this timestamp conversion fix across all 3 platforms or wrappers but I guess the arguments for doing it in libSession are:

  1. Isn't one of the points of libSession to help reduce duplicating work across platforms
  2. There are a bunch of other inputs that libSession validates so it makes sense in my mind for it to validate that the durations provided are correct (ie. in seconds), and if not valid then automatically convert them when possible (eg. similar to how we auto-truncate strings to the max lengths in some cases)

Since some users will already have configs which have invalid timestamps just throwing when given an invalid value wouldn't help those users and may result in unexpected crashes (we introduced a crash on Desktop when originally adding exceptions for the name length requirement in UserProfile) hence the conversion approach

If we are able to get some more time to dedicate to libSession next year I'm hoping we can start to move more of the business logic to the libSession side so that these types of bugs become less likely (eg. if there were createGroup(name:) & joinGroup(ed25519PubKey:authData:) functions then the timestamps wouldn't need to be set directly by the frontends)

@jagerman
Copy link
Member

This is unfortunately less of a nodejs-specific thing and more an attempt to fix some libSession misuse that happened at some point - there was some invalid conversions on one or more of the platforms which resulted in incorrect timestamps being added into libSession which, unsurprisingly, caused a bunch of bugs

I see. Yeah, if that broken data is out there in the wild then we need to deal with it, and here is the place that makes sense (sort of like the protobuf double-wrapping issue).

Are current (or in-progress) clients now passing the correct value, so that we can treat this as a workaround for past issues?

If we are able to get some more time to dedicate to libSession next year I'm hoping we can start to move more of the business logic to the libSession side so that these types of bugs become less likely (eg. if there were createGroup(name:) & joinGroup(ed25519PubKey:authData:) functions then the timestamps wouldn't need to be set directly by the frontends)

100% yes, and that is indeed on the agenda for starting right away in 2025.

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.

3 participants