Details
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Worth adding to the Summary or Test Plan that this includes all RPC (all usages of getRustAPI) other than unath RPCs.
Ideally, I would see this improved and have this typed like IdentityServiceClient, and to be lazy initialized when we e.g. call getRustAPI only once, I think those stateless functions are hard to maintain, but this is not feedback to this diff, we can create a follow-up task to consider this if you agree.
| keyserver/src/utils/identity-utils.js | ||
|---|---|---|
| 22–24 | shouldn't we use include()? | |
Good point, this might not be immediately obvious.
Ideally, I would see this improved and have this typed like IdentityServiceClient, and to be lazy initialized when we e.g. call getRustAPI only once, I think those stateless functions are hard to maintain, but this is not feedback to this diff, we can create a follow-up task to consider this if you agree.
I have drafted an implementation that proxies RustAPI object and wraps every promise call within a try-catch. Finally I considered it an overkill but that would solve your concern
| keyserver/src/utils/identity-utils.js | ||
|---|---|---|
| 22–24 | For Identity RPCs we have the Display trait implemented (derived) on Rust side which exactly displays the error code. I used includes() only for Tunnelbroker error traits because their Display is implemented more implicitly and also shared between client and server, so more likely to change in the future. Using includes there made sense to make it more future-proof. Here, the Display trait is right in rust-node-addon, so there's no need. | |