feat(wallet-gateway-remote): added connectivity status to login #1003
Conversation
Signed-off-by: phillip olesen <[email protected]>
Signed-off-by: Phillip Olesen <[email protected]>
Signed-off-by: phillip olesen <[email protected]>
|
@PHOL-DA I had a think about persistence/scalability. I agree, but this should not go into our domain models, but instead in a separate 'cache'. |
Signed-off-by: phillip olesen <[email protected]>
Signed-off-by: phillip olesen <[email protected]>
Signed-off-by: phillip olesen <[email protected]>
| "VerifiedNetwork": { | ||
| "title": "VerifiedNetwork", | ||
| "description": "Structure representing the Networks with connectivity verification", | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/components/schemas/Network" | ||
| }, | ||
| { | ||
| "title": "VerifiedNetworkProperties", | ||
| "description": "holder for boolean verified property", | ||
| "type": "object", | ||
| "properties": { | ||
| "verified": { | ||
| "title": "verified", | ||
| "description": "true if we could succesfully call the ledger version endpoint", | ||
| "type": "boolean" | ||
| } | ||
| }, | ||
| "required": ["verified"] | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
why does this need a separate schema, rather than adding an optional "verified" to Network itself?
| cron.schedule('* * * * *', () => { | ||
| startLedgerConnectivityTester(networkCacheStore, logger) | ||
| }) | ||
| } |
There was a problem hiding this comment.
I think this runs every minute? is that too frequent?
| export class NetworkCacheStore { | ||
| constructor( | ||
| private readonly store: Store, | ||
| private readonly logger: Logger | ||
| ) {} |
There was a problem hiding this comment.
Do you need all this? I'm wondering why not decouple the cache from the underlying store?
The network cache can just store { networkId: string, verified: boolean }, and only needs a getter and setter.
In the controller you retrieve the network from the store directly, and merge based on ID to include the verified status
There was a problem hiding this comment.
if we wanted to go all out, we might even feel inclined to design a Cache interface analogous to Store. Then down the road we could have an in-memory cache implementation, and then something like Redis implementation for a HA/scalable production setup
mjuchli-da
left a comment
mjuchli-da
left a comment
There was a problem hiding this comment.
As mentioned in the comments, I believe it would be better to hold only the cached properties (status in this case) in the LRU. Then use this cache here needed: which is only when listing networks. This way, we have much better decoupling between the domain model (Network) and a (single) cached property (Status) that is not part of the domain model.
| networks: z.array(networkSchema), | ||
| }) | ||
|
|
||
| export const verifiedNetworkSchema = networkSchema.extend({ |
There was a problem hiding this comment.
Hm, I don't think this has anything to do with the wallet-store. Can we define it directly in the wallet-gateway?
| "type": "string", | ||
| "description": "The unique identifier of the command associated with the transaction." | ||
| }, | ||
| "VerifiedNetwork": { |
There was a problem hiding this comment.
The term Verified is a bit ambiguous since we use it to indicate a "verified" wallet in the SDK.
What this really indicates is the "availability", no?
There was a problem hiding this comment.
hahaha, I just had the same comment. Didn't think about availability though, I like that the most!
EDIT: or plain available
| export const DEFAULT_MAX_CACHE_SIZE = 100 | ||
| export const DEFAULT_ENTRY_EXPIRATION_TIME = 10 * 60 * 1000 | ||
|
|
||
| const NetworkCache = new LRUCache<string, VerifiedNetwork>({ |
There was a problem hiding this comment.
Why not simply cache the status for each network NetworkStatusCache = LRU<NetworkId, NetworkStatus>?
With
NetworkStatus = { connected: boolean, reason?: string, lastChecked: Date }
This way, when listing existing networks, nothing needs to change, except for an additional cache lookup to get the status.
| entryExpirationTimeInMS: DEFAULT_ENTRY_EXPIRATION_TIME, | ||
| }) | ||
|
|
||
| export class NetworkCacheStore { |
There was a problem hiding this comment.
I don't see the benefit of this. I'd separate concerns and only cache the NetworkStatus (or an alternative name: NetworkAvailability).
|
|
||
| // TODO: Add an explicit updateNetwork method to the User API spec and controller | ||
| const existingNetworks = await store.listNetworks() | ||
| const existingNetworks = await cache.listNetworks() |
There was a problem hiding this comment.
We should not have to touch the cache here. See my suggestion for separating things above.
| }, | ||
| removeNetwork: async (params: RemoveNetworkParams) => { | ||
| await store.removeNetwork(params.networkName) | ||
| await cache.removeNetwork(params.networkName) |
| listNetworks: async () => | ||
| Promise.resolve({ networks: await store.listNetworks() }), | ||
| listNetworks: async () => { | ||
| return { networks: await cache.listNetworks() } |
There was a problem hiding this comment.
Here we can list the networks, then in addition, get the status via the cache.
Motivation:
This PR introduces background runners and caching to the wallet gateway that allows us to (in memory) continuously maintain the status of ledger endpoints before authentication. This will become a lot handier further down the road when we split idp and network completely.
considerations:
as @mjuchli-da have mentioned in comments for persistence and scaling reason this should be in cache / memory only.
for now this is a proof of concept that delivers also a short term value (in the form of the status icon), that creates a basis for further similar works.
With nothing running locally:
Screencast from 2025-12-11 08-48-10.webm
With
start:canton&start:localnetScreencast from 2025-12-11 08-51-15.webm