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

Use int64 for API timestamp #500

Closed
wants to merge 2 commits into from

Conversation

G10h4ck
Copy link
Contributor

@G10h4ck G10h4ck commented Mar 2, 2022

Address #345

@G10h4ck
Copy link
Contributor Author

G10h4ck commented Mar 2, 2022

Seems we have similar but unrelated problem in ipfsync too https://github.com/vocdoni/vocdoni-node/blob/master/ipfssync/ipfssync.go#L40 @altergui I guess that should be treated separately

@G10h4ck G10h4ck force-pushed the api_timestamp_int64 branch from dfc5f4f to 4205624 Compare March 2, 2022 10:05
@altergui
Copy link
Contributor

altergui commented Mar 2, 2022

Seems we have similar but unrelated problem in ipfsync too https://github.com/vocdoni/vocdoni-node/blob/master/ipfssync/ipfssync.go#L40 @altergui I guess that should be treated separately

right, but in ipfssync, AFAIU, there's no api involved, the messages are only passed between ipfssync instances. so maybe it's a non-issue?

@G10h4ck
Copy link
Contributor Author

G10h4ck commented Mar 2, 2022

right, but in ipfssync, AFAIU, there's no api involved, the messages are only passed between ipfssync instances. so maybe it's a non-issue?

If you plan to make it obsolete before 2037 no problem ;)

@G10h4ck G10h4ck changed the title WIP: Use int64 for API timestamp Use int64 for API timestamp Mar 8, 2022
@jordipainan
Copy link
Member

LGTM

@marcvelmer @brickpop @emmdim I think this also affects other repos can we sync on this ?

@G10h4ck
Copy link
Contributor Author

G10h4ck commented Mar 10, 2022

I have made a PR for dvote-js too vocdoni/dvote-js#99

@NateWilliams2
Copy link
Contributor

@G10h4ck Why are you using the json string tag for these fields? Is it needed?
I'm getting an error: getInfo: json: invalid use of ,string struct tag, trying to unmarshal unquoted value into int64

@G10h4ck
Copy link
Contributor Author

G10h4ck commented Mar 11, 2022

@G10h4ck Why are you using the json string tag for these fields? Is it needed? I'm getting an error: getInfo: json: invalid use of ,string struct tag, trying to unmarshal unquoted value into int64

Are you getting that error attempting to parse a JSON generated by older version with the new one?
Using string representation for 64 bit integers is necessary to make it properly parse-able by clients that doesn't natively supports 64 bit integers natively such as Javascript and Dart

@p4u
Copy link
Member

p4u commented Mar 14, 2022

Jumping in without full context. As @G10h4ck states in the issue, there could be a precission problem if using int64 when interacting with JS libraries, please do not merge this until we are sure we won't have interoperability problems: @mvdan @brickpop @marcvelmer ?

In the worst case we can let it unmerged since the RPC API will require a full refactor in some moment.

@mvdan
Copy link
Contributor

mvdan commented Mar 14, 2022

I thought we were fine with this small API breaking change, given that we can update the clients too at the same time. If we want the transition to be smoother, the server could accept both forms when decoding by implementing json.Unmarshaler. I can help with that if it sounds useful. We would still always encode as a string in the new version, so this would only help with older requests that didn't use strings, it won't help with clients that expect non-string responses.

@p4u p4u closed this Oct 6, 2022
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.

6 participants