CIP #5
CIP #5
Conversation
|
cc: @rhyslbw |
There was a problem hiding this comment.
Great start @will-break-it.
- I suggest focusing on the CIP now in this repo, so can drop
02-Synchronisation.pumlto remove the duplication. The generated outputdocs/02-Synchronization-Sequence.svgis also out of date since it has the async projection, whereas it's been hidden in the source file.
- One of the traditional operational challenges of serving stateful clients is session management during times of scaling. I think it's worth specifying the server can disconnect clients at any time (since they can just reconnect and resume)
There's some open questions unanswered in the CPS but I think we can worth responses in after doing some more prototyping, so let's just look to lock down a first version here and then we can come back to it.
docs/CIP/CIP-XXXX/README.md
Outdated
|
|
||
| - [`heartbeat`](./messages/client/heartbeat.md): Connection management/ keep alive | ||
| - [`subscribe`](./messages/client/subscribe.md): Define topics of interest & authentication | ||
| - [`unsubscribe`](./messages/client/unsubscribe.md): Graceful disconnect from server |
There was a problem hiding this comment.
I see this as an useless message which only effect is to increase the complexity without advantages.
- I don't see a real use case where a wallet doesn't want to stop following transactions for its addresses.
- Most likely clients may choose to simply disconnect without caring about this: being no advantages for them in implementing this, there is an high probability many companies will prefer to save money to implement it.
- The server needs to unsubscribe the client on disconnect anyway:
- to protect from unexpected close,
- to not waste resources for lazily wrote clients which simply disconnect.
I'd simply remove it.
There was a problem hiding this comment.
@iccicci If you have ten accounts in your wallet and want to remove one of them, you might want to be able to unsubscribe.
There was a problem hiding this comment.
Yes @ginnun , in theory yes... but which is the use case?
Since others can still send transactions to addresses bound to that account, what does it means "I want to remove an account"?
To me sounds as "if somebody still send me coins on one of these addresses, I'm no longer interested: it's fine if I do not track them".
When I want to stop using an account, I'll stop sharing addresses bound to it with other to ask for payments, but I'm still interested (much interested) in transactions which give me something.
What am I missing?
Thank you
There was a problem hiding this comment.
You are still the account owner, but you are no longer interested in monitoring it on a given subscription. One person can have multiple installations of the wallet, e.g., mobile and desktop, simultaneously.
A wallet will probably allow you to remove a wallet address via the UI (Lace does), why should server still send events related to it?
- After some time, you may want to decrease the number of subscriptions in your mobile wallet while keeping everything active on the desktop. You may want to hide it for some reason.
- You may want to drop an unused subscription because you want to.
There was a problem hiding this comment.
Makes sense...
Probably we can just mark it as optional for the clients.
docs/CIP/CIP-XXXX/README.md
Outdated
|
|
||
| The `signature` field is generated by signing with the [extended account private key](https://input-output-hk.github.io/adrestia/static/Ed25519_BIP.pdf) a SHA256 HMAC hashed string. The prehash string is constructed by concatenating the subscribed blockchain(s) + timestamp. | ||
|
|
||
| The server verifies the signature and timestamp before serving any data to the client. If any part of the `subscribe` message is invalid, the server sends an error and closes the connection. |
There was a problem hiding this comment.
Should we mention a timestamp window since we will use timestamp for signature verification and try to guess it on the server? Should the timestamp be in a 60ms window, meaning the server tries 60 times before failing?
|
|
||
| Address encoding varies depending on the blockchain network. In Cardano the protocol servers bech32 encoded addresses. | ||
|
|
||
| ### Extensions |
There was a problem hiding this comment.
We need to define how extensions will be supported under this standard. Best to take inspiration from cardano-foundation/CIPs#462.
docs: remove old fields from subscribe message
There was a problem hiding this comment.
I left a review on @ginnun's PR, but it really applied here instead. I didn't pick this up in my first review.
docs: add AsyncAPI definition
docs: remove docs folder
Co-authored-by: Rhys Bartels-Waller <rhys.bartelswaller@iohk.io>
Rendered Version