Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nip-86: updating methods, adding new ones. #1734

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kehiy
Copy link
Contributor

@kehiy kehiy commented Feb 1, 2025

read parsed version.

these changes are based on discussions on: mikedilger/chorus#39.

and methods needed by the mangostr client: https://github.com/dezh-tech/mangostr.

method names are used to use snake_case since they are much more readable.

@kehiy
Copy link
Contributor Author

kehiy commented Feb 1, 2025

@mikedilger @fiatjaf fyi.

if you think this is ok, i will work on the chorus and khatru implementation myself.

Copy link
Contributor

@mikedilger mikedilger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't spend time checking this, just noticed this one thing.

@mikedilger
Copy link
Contributor

@fiatjaf notice names changed to snake_case so anything that is out there already will break, but Kehiy seems keen to code this in both our relays. Personally I prefer snake_case to mashedcase.

@kehiy
Copy link
Contributor Author

kehiy commented Feb 2, 2025

I didn't spend time checking this, just noticed this one thing.

i think the one thing was missed when you submitted the review. am i right? 👀

@kehiy kehiy requested a review from mikedilger February 2, 2025 12:00
@fiatjaf
Copy link
Member

fiatjaf commented Feb 2, 2025

I don't know what methods are being added because you reformatted the entire thing. I prefer the old formatting.

@kehiy
Copy link
Contributor Author

kehiy commented Feb 2, 2025

you can check the parsed version and then we can talk about protocol changes, later i will update the formatting to the old one. its a little time-consuming and im not able to update it atm.

@fiatjaf

@kehiy
Copy link
Contributor Author

kehiy commented Feb 3, 2025

@mikedilger @fiatjaf ive updated the formatting.

86.md Outdated Show resolved Hide resolved
@kehiy kehiy requested a review from staab February 3, 2025 14:52
@mikedilger
Copy link
Contributor

I think the changes are like this (this took a while to figure out):

HIGH LEVEL THINGS
	* supportedmethods	RENAMED supported_methods
	* changerelayname  	REMOVED	
	* changerelaydescription REMOVED
	* changerelayicon 		REMOVED
	* ADDED: stats
	* ADDED: set_nip11
	* ADDED: grant_admin
	* ADDED: revoke_admin

PUBKEY-based
	* banpubkey 			RENAMED TO ban_pubkey
	* listbannedpubkeys 	RENAMED TO list_banned_pubkeys
	* allowpubkey 		RENAMED TO allow_pubkey
	* listallowedpubkeys 	RENAMED TO list_allowed_pubkeys

EVENT-based
	* listeventsneedingmoderation	RENAMED to list_events_needing_moderation
	* allowevent 			RENAMED TO allow_event
	* banevent 			RENAMED TO ban_event
	* listbannedevents 		RENAMED TO list_banned_events
	* ADDED list_allowed_events

KIND-based
	* allowkind 			RENAMED allow_kinds
	* disallowkind 			RENAMED disallow_kinds
	* listallowedkinds 		RENAMED list_allowed_kinds
	* ADDED list_disallow_kinds

IP-based
	* blockip 				RENAMED block_ip
	* unblockip 			RENAMED unblock_ip
	* listblockedips 		RENAMED list_blocked_ips

@mikedilger
Copy link
Contributor

list_allowed_events seems like it would produce too much data, almost every event on the relay.

list_disallow_kinds should be renamed list_disallowed_kinds

@kehiy
Copy link
Contributor Author

kehiy commented Feb 4, 2025

@mikedilger thank you for summarizing the changes! yes, they are correct.

and i agree that list allowed events is a little weird. my idea was when you have a relay where all events are disallowed by default. like the idea behind white listing. but it would have a very limited use-case, we can remove it as well.

@kehiy kehiy closed this Feb 4, 2025
@kehiy kehiy reopened this Feb 4, 2025
@kehiy
Copy link
Contributor Author

kehiy commented Feb 4, 2025

list_disallow_kinds is also renamed to list_disallowed_kinds.

@kehiy
Copy link
Contributor Author

kehiy commented Feb 5, 2025

any updates guys?

@fiatjaf
Copy link
Member

fiatjaf commented Feb 5, 2025

These renames are all completely unnecessary.

I'm fine with adding grantadmin and revokeadmin. stats I don't think we should have because each relay will have different sets of stats so it's better if each does its own thing outside of the scope of this NIP -- or at least we need more experience before we standardize it. setnip11 doesn't make any sense.

@kehiy
Copy link
Contributor Author

kehiy commented Feb 5, 2025

@fiatjaf i believe snake_case is much more readable. why not use it?

the stat command is just a base definition here. relays can add/remove fields to respond based on their design. fields are not restricted.

the set_nip11 command helps us manage nip11 and configs easily. otherwise, we have to add a huge amount of dedicated commands for each nip11 field and then we don't support custom fields.

@fiatjaf
Copy link
Member

fiatjaf commented Feb 6, 2025

@fiatjaf i believe snake_case is much more readable. why not use it?

Because it's a completely unnecessary breaking change. Also I don't believe it is much more readable.

the stat command is just a base definition here. relays can add/remove fields to respond based on their design. fields are not restricted.

There is no point in having a specification that doesn't specify anything.

the set_nip11 command helps us manage nip11 and configs easily. otherwise, we have to add a huge amount of dedicated commands for each nip11 field and then we don't support custom fields.

I said it doesn't make sense because relays have these fields mostly hardcoded, generated based on other properties or configured by config files or environment variables. It doesn't make sense to change these things from an RPC command. The relay would then have to save that information and then act on it? How? It would be a mess. I'm not saying it wouldn't be doable or perhaps even useful in some cases, but in practice this is a confusing hassle that no one will use and will just bloat the spec.

@mikedilger
Copy link
Contributor

the stat command is just a base definition here. relays can add/remove fields to respond based on their design. fields are not restricted.

There is no point in having a specification that doesn't specify anything.

Kehiy started with a bunch of separate well defined stat commands. But I know what will happen in practice is that there will be a 5 second lag while clients request each stat one at a time until they get them all. It doesn't make sense to do lots of round trips for individual statistics. Basically the meaning of the stats fields probably don't carry much meaning for software to act upon, the whole blob can just be displayed to the end user.

@fiatjaf
Copy link
Member

fiatjaf commented Feb 6, 2025

Basically the meaning of the stats fields probably don't carry much meaning for software to act upon, the whole blob can just be displayed to the end user.

In this case I like it.

@kehiy
Copy link
Contributor Author

kehiy commented Feb 6, 2025

@mikedilger thanks a lot for explanation.

@fiatjaf i can turn back the name cases, but i still prefer snake case.

also, i believe nip11 setting is a better replacement for one command per field like setname. same as one command per stat field it would be annoying.

but i will remove setnip11 and update naming.

@kehiy
Copy link
Contributor Author

kehiy commented Feb 6, 2025

@mikedilger @fiatjaf spec updated. nip11 command removed and naming is same as before.

@mikedilger
Copy link
Contributor

I also like snake case, but I don't like breaking things, and I expected this outcome which is why I never tried to change the names.

I didn't scour it again, but it looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants