Introduce status method allowing to query the Node's status #272
Introduce status method allowing to query the Node's status #272tnull merged 3 commits intolightningdevkit:mainfrom
status method allowing to query the Node's status #272Conversation
tnull
force-pushed
the
2024-03-add-node-status
branch
5 times, most recently
from
e317d13 to
4dd7ea7
Compare
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct BestBlock { |
There was a problem hiding this comment.
Is this only needed because LDK's struct doesn't derive Debug?
There was a problem hiding this comment.
It not deriving Debug is one reason, but more importantly it doesn't expose the block_hash/height fields directly, only via methods. This is fine in Rust, but for our bindings it would mean that we'd need to expose it as an interface rather than a dictionary, in turn meaning NodeStatus would need to hold an Arc<BestBlock>, etc.
So it's easiest here to introduce a newtype that's compliant with the limitations of our bindings generator.
There was a problem hiding this comment.
I now opened lightningdevkit/rust-lightning#2923, but would still prefer to go ahead with the newtype wrapper in this PR and switch to using the refactored upstream version in #243 to allow this PR to land independently.
| pub latest_wallet_sync_timestamp: Option<u64>, | ||
| /// The timestamp, in seconds since start of the UNIX epoch, when we last successfully synced | ||
| /// our on-chain wallet to the chain tip. | ||
| /// | ||
| /// Will be `None` if the wallet hasn't been synced since the [`Node`] was initialized. | ||
| pub latest_onchain_wallet_sync_timestamp: Option<u64>, | ||
| /// The timestamp, in seconds since start of the UNIX epoch, when we last successfully update | ||
| /// our fee rate cache. | ||
| /// | ||
| /// Will be `None` if the cache hasn't been updated since the [`Node`] was initialized. | ||
| pub latest_fee_rate_cache_update_timestamp: Option<u64>, | ||
| /// The timestamp, in seconds since start of the UNIX epoch, when the last rapid gossip sync | ||
| /// (RGS) snapshot we successfully applied was generated. | ||
| /// | ||
| /// Will be `None` if RGS isn't configured or the snapshot hasn't been updated since the [`Node`] was initialized. | ||
| pub latest_rgs_snapshot_timestamp: Option<u64>, | ||
| /// The timestamp, in seconds since start of the UNIX epoch, when we last broadcasted a node | ||
| /// announcement. | ||
| /// | ||
| /// Will be `None` if we have no public channels or we haven't broadcasted since the [`Node`] was initialized. | ||
| pub latest_node_announcement_broadcast_timestamp: Option<u64>, |
There was a problem hiding this comment.
Would it make sense to use Duration for these?
There was a problem hiding this comment.
No, if anything we'd want to make them SystemTime (or chrono::DateTime for that matter), but we'll need to convert to timestamps for the bindings anyways. Same goes for the upcoming inclusion of a last_changed timestamp in the PaymentDetails, where an additional blocker is that we don't implement Readable/Writeable for SystemTime 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.
Hmm... Duration is a timestamp when interpreted as relative to the Unix epoch. It just gives you more precision than u64 seconds and may be more useful interface to work with. But fair enough regarding bindings.
There was a problem hiding this comment.
Ah, yeah, I agree in Rust a Duration would be preferable and I see that we even implement Writeable/Readable for it. However, in the bindings it would be weird because I can either translate it to a u64 where it becomes unclear what unit to use: in this context seconds would make sense, but otherwise we might want to translate it to a u64 representing nanos or so. The other way would to expose it as an interface Duration that mirrors the Rust type. However, in Kotlin/Swift/Python introducing a general Duration 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.
jkczyz
left a comment
jkczyz
left a comment
There was a problem hiding this comment.
LGTM. Seems like you'll need a rebase.
.. we replace the simple `is_running` with a more verbose `status` method returning a `NodeStatus` struct, giving more information on syncing states etc.
tnull
force-pushed
the
2024-03-add-node-status
branch
from
4dd7ea7 to
b052f15
Compare
|
Rebase without further changes. |
2 participants
Fixes #127.