-
Notifications
You must be signed in to change notification settings - Fork 14
Conversation
specs/waku/mailserver.md
Outdated
- **from [int]** - `required` - UNIX time in seconds; oldest requested envelope's creation time. | ||
- **to [int]** - `required` - UNIX time in seconds; newest requested envelope's creation time. | ||
- **bloom [string]** - `optional` - array of Waku topics encoded in a bloom filter to filter envelopes. | ||
- **limit [int]** - `required default: 100` - The maximum amount of envelopes to return. |
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.
Could we lift the required
if we have a default?
specs/waku/mailserver.md
Outdated
- **to [int]** - `required` - UNIX time in seconds; newest requested envelope's creation time. | ||
- **bloom [string]** - `optional` - array of Waku topics encoded in a bloom filter to filter envelopes. | ||
- **limit [int]** - `required default: 100` - The maximum amount of envelopes to return. | ||
- **page [int]** - `optional` `default: 0` - The page to return. |
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.
Here we want likely to use cursor
based pagination instead of page
based, as page based is not stable.
Essentially mailserver will return a cursor
(string) which is returned in the response, and used to fetch the next "page"
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.
https://jsonapi.org/profiles/ethanresnick/cursor-pagination/ for reference, the current mailserver implementation already uses a cursor
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.
good point, thanks @cammellos
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.
changed @cammellos
specs/waku/mailserver.md
Outdated
"nonce": "0x0123", | ||
"data": "0x129232138ae123", | ||
}], | ||
"page": 0, |
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.
This probably we would want it to be a cursor instead. Other api's offer a next
and previous
URL, which is also neat, but we currently only fetch using previous
, so maybe not needed for now.
specs/waku/mailserver.md
Outdated
"data": "0x129232138ae123", | ||
}], | ||
"page": 0, | ||
"count": 1, |
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.
We probably don't need count (although it does not harm as there's no performance issue), nor pages (pages might have to be calculated, and it might be expensive depending on implementation)
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.
removed
Just thinking out loud here - does it have to be a REST API? Not clear to me that resources abstraction map neatly. E.g. we only do GET here, no POST or UPDATE afaict. It also restricts us to HTTP. Would gRPC or JSON RPC (over HTTPs to start with, but extensible to e.g. WS and some direct kad p2p in future) be interesting? The main thing we want here AFAIK is request-response. |
The problem with gRPC, from what I hear is that its browser support is trash. I like the idea of thinking whether it needs to be rest @oskarth, my rationale for it was that it seems like a good radical change for getting away from json rpc. I would not be opposed to moving entirely away from JSON RPC in the distant future. What I will define is a protobuf service, that way we can abstract away the transport and serialization too and maybe add support for multiple? |
Relevant discussion ethereum/consensus-specs#1012 |
So as discussed with @oskarth, the play here seems to be that we should firstly define domain objects using protocol buffers along with the service. Once that is done we can start a discussion on the transport looking at various trade-offs, such as privacy, interaction with current tech, ease of implementation. |
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.
The way that I understood this issue is that the main problem is that the current mail server specification does not make use of the request/response mechanism (following packet ids) in the waku protocol.
We can extend it with an HTTP interface also if there is a need for this, but I'd like to hear the reasoning of why there is the need. Sorry if I missed this in earlier convo.
If we add this HTTP (REST) interface, I agree that ideally the definition of the objects is (or becomes) the same for this HTTP REST interface, and the existing Waku one (which will then requires changes probably). Serialisation will/can of course be different.
@kdeme status-im/specs#54 (comment) specifically for HTTPS. |
What would be the minimal change for us to get request-response? I'm a bit worried that if we start to get too much into the comfy REST/HTTP world it'll be hard to get rid of it, and we'll start to rely increasingly on cluster, DNS, load balancing, etc. |
Essentially the main problem is the maximum message size, which is the reason why they are sending them separate from the response (we split envelopes in batches, and we fit as many in the batch as we can). The problem boils down to, "how do you send an envelope where size(envelope) == maxMessagesize?". The only way to do that is to send the envelope without any metadata, if you want to respect the maxMessageSize (which is checked before doing any parsing, rightly so as it might choke the parser an oversized message). Without increasing the max message size (which is probably not a good idea) we won't be able to have proper req/response, and you will either have to piece things up on one end (which we don't do currently, but we could at the expenses of complexity), or get into something of a more concerted way to send envelopes when syncing between two peers (so one peer will have expectations about what the next message should look like once a request is made) . HTTP does not have this problem as it's polling rather than pushing, so lots of complications go away, but it's true that it's quite a change. From purely a core perspective, this only matters once we have bandwidth to implement it, and currently mailserver is probably not the highest priority as there's not much appetite in changing things as it mostly works. This would give us some time to sit on the decision for a bit longer. |
@oskarth, I see your point. But the way I say it the mailserver is a construction we want to eliminate later anyway. We might as well make this a REST based api and ensure that later storage protocols etc interact nicely without the need for such an API. Does that make sense? |
Yes, I have the same concern which is why I asked why we really need to go this route. Sure, a lot of complications go away with HTTP, but that is not the point. Regarding the Regarding the specification here, can't we just go with ABNF specification? And then we can keep RLP in the current RLPx solution. And move to something else, e.g. protobuf, whenever we decide to move to libp2p or any other tech. |
You have a point. The mailserver is already a centralized concept so you could say, lets go all the way then. From the perspective of the Nim implementation, I don't think we will be able to do the effort to support the HTTP solution, as we don't have a proper HTTP(S) server implementation. But I guess that is fine as we don't have the current mail server now anyhow, and no real reason to need one. Building the Nim mailserver as it is defined now is actually also rather low in priority list (if it is even on it :) ). So, I'll leave it to the status-go team to decide the best way forward. |
@kdeme it seems like it may make sense that a client can determine their message says and send to a peer what message size they want? |
Seems outdated and better taking lessons from here and applying it to v2. Close? |
@oskarth yea |
This PR introduces a new REST API for interacting with mailservers as documented in vacp2p/research#24.
This PR mainly serves for discussion and is no where near final. There are certain things that still need to be discussed.
80
,8080
or443
?Todos