Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduce
status
method allowing to query theNode
's status #272Introduce
status
method allowing to query theNode
's status #272Changes from all commits
84ecc24
0396fc5
b052f15
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to use
Duration
for these?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, if anything we'd want to make them
SystemTime
(orchrono::DateTime
for that matter), but we'll need to convert to timestamps for the bindings anyways. Same goes for the upcoming inclusion of alast_changed
timestamp in thePaymentDetails
, where an additional blocker is that we don't implementReadable
/Writeable
forSystemTime
upstream.So I currently lean towards using UNIX timestamps in the API as everything else seems to complicate things further?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm...
Duration
is a timestamp when interpreted as relative to the Unix epoch. It just gives you more precision thanu64
seconds and may be more useful interface to work with. But fair enough regarding bindings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, I agree in Rust a
Duration
would be preferable and I see that we even implementWriteable
/Readable
for it. However, in the bindings it would be weird because I can either translate it to au64
where it becomes unclear what unit to use: in this context seconds would make sense, but otherwise we might want to translate it to au64
representing nanos or so. The other way would to expose it as aninterface Duration
that mirrors the Rust type. However, in Kotlin/Swift/Python introducing a generalDuration
type wouldn't make a lot of sense. Technically Uniffi would have the capability to translate a value on the bindings side once more, which would allow us to use the idiomatic Swift/Kotlin/Python types. I'll think about it some more. Good thing is that these fields aren't persisted, i.e., until we do we can easily reconsider/improve them.