ircv3 / ircv3-ideas

46 stars 3 forks source link

Standardise client-bouncer interactions #39

Open DanielOaks opened 5 years ago

DanielOaks commented 5 years ago

This is something that's been in the works for a fairly decent while now, and has been pretty consistently recommended as something we should work on. This issue's mostly a note that "yea this is being worked on".

For prior art on the stuff currently being worked on, here's a few links: https://gist.github.com/prawnsalad/d864822d9bb8116626a4370bee6e6fc6 https://www.irccloud.com/pastebin/IMi5OozE/ https://github.com/goshuirc/bnc/blob/master/lib/components/bouncer/bouncer.go

grawity commented 5 years ago

should the canonical form of subcommands be capitalised?

IIRC, all verbs in IRC are uppercase, so imho this should match existing specs (CAP uses uppercase and iirc ACC does as well; did I forget some which don't?)

changenetwork - The tags may only include 1 or all the tags

How do you "unset" a parameter (e.g. make it inherit from global default where applicable)? Send a null tag (without "=value")?

listnetworks is the only command that doesn't have a netname after it, for obvious reasons. but is this an annoying inconsistency that will have to be special cased. should we spec a * to be sent with that command, even if that seems redundant?

Specifying a * to be sent seems perfectly fine to me – just pretend that it's a wildcard for filtering the list output, like in IMAP or "/msg chanserv list". (Bouncers may implement wildcard-like filtering or may always return the full list as if * was sent.)

prawnsalad commented 5 years ago

I've updated the gist with the pastebin changes. Not in spec form yet, just getting all the data together to be worked formally through after.

@grawity, unsetting via a null tag makes sense. I'll add that in.

  1. I'm not sure I see the need for the extra * param after listnetworks if it has no function. Would simple wildcard searching ever be needed for listing networks? I guess some clients could organise networks with a prefix and use wildcard to browse them... a possible use case.

  2. I'm planning to make use of the new FAIL/WARN/etc responses rather than the custom error structure currently described in the gist.

  3. Passthrough command to send raw data to the IRCd I like the idea for this, especially for debugging purposes, but a few thoughts come to mind with this one. @jwheare you might have some thoughts on this one as you originally brought up this idea

    • Will this conflict with the bouncer implementation? ie. Sending CAP LS will cause the bouncer to handle the replies.
    • Should the bouncer attempt to return the raw response from the IRCd or should it wrap it. This could be difficult and messy to handle
grawity commented 5 years ago

unsetting via a null tag makes sense

Sigh, I just remembered that the message-tags spec says "specifications must not assign different meanings to empty and null" for compatibility reasons. Oh well.

prawnsalad commented 5 years ago

Gist updated with feedback, https://gist.github.com/prawnsalad/d864822d9bb8116626a4370bee6e6fc6

@jwheare I've still been thinking on how to handle the passthrough. I'm debating dropping it as I can only see issues with it as mentioned in the gist.

jwheare commented 5 years ago
prawnsalad commented 5 years ago

BOUNCER=network= is the freeform name the user has saved the network under, not the name the network itself provides. Eg. I may want 2 connections to freenode under the same bouncer, but one called freework and one called freesocial.

For the standard replies, it does make it more awkward to parse but if these replies are to become standard going forward then it would be best to start using them when we can IMO.

SadieCat commented 5 years ago

This would probably be easier to review as a pull request.

prawnsalad commented 5 years ago

@SaberUK probably. I've been getting the general syntax down roughly first before typing it as a draft otherwise it would take forever to switch between language when large portions are changed. Once discussion around the replies format have happened then its probably a good time to do that.

DarthGandalf commented 5 years ago

One case to consider: if there is a chain of connected bouncers, how would client know which bouncer in the chain is talking to it. Or which of them is sending BOUNCER=network=

prawnsalad commented 5 years ago

@DarthGandalf unless we get some sort of multi layered protocol going on, I'm not sure chained bouncers would be in scope here? It would definitely complicate a lot of things

jwheare commented 5 years ago

Well, each bouncer would have to negotiate the cap separately before receiving it, and bouncers only selectively pass through commands, like e.g. CAP so the client is only going to get a response from the last one in the chain, no?

prawnsalad commented 5 years ago

@DanielOaks as you're the author of the standard replies, what are your thoughts on the usage of them here? IMO using them here makes it a little more awkward than it could be and also mixes bouncer + standard replies... replies. However if we are to be standardising on replies going forward then it makes sense to start using them, and this shows that the current set of standard replies isn't working so well as yet.

Thoughts?

DanielOaks commented 5 years ago

Honestly I'm having a tough time thinking about how to make them better for use in this. Like... if you wanted to you could maybe send them as subcommands instead (e.g. BOUNCER FAIL and BOUNCER NOTE instead), but really I don't see the primary verbs being BOUNCER and FAIL as that big of a deal (hell, with regular numerics a million different commands can receive the ERR_NOSUCHNICK verb in response, BOUNCER does better off the bat).

Might be worth recommending an order for the contexts here, or wherever standard replies are used to reply to a command that makes use of subcommands, like:

FAIL/NOTE COMMAND ERROR_CODE SUBCOMMAND [other info] DESCRIPTION

Making this a standard, recommended format for responding to commands like BOUNCER feels like it could be useful. We can't change the format around and e.g. put the optional subcommand before the error code because there's no way to note that with the variable list of contexts.

DanielOaks commented 3 years ago

As part of our playing with push notifications, we're needing to define a way for clients to notify (bouncers, or the equivalent that is our server in our case) of when certain clients are active or not. We're messing around with it over here: https://github.com/oragono/oragono/issues/1551 (doesn't really conflict with the BOUNCER stuff talked about above in my mind, the first comment of the linked issue explains my thoughts around it).

There's also a little issue that clients have when connecting to bouncers (and just the same, our server's persistence implementation) around join messages when rejoining. I'm mostly collecting ideas in this issue, but since it'd also be relevant for bouncers I'll link it here too: https://github.com/oragono/oragono/issues/1548