paritytech / json-rpc-interface-spec

29 stars 3 forks source link

Remove `archive_genesisHash`, since we have `chainSpec_V1_genesisHash` already? #101

Closed jsdw closed 1 year ago

jsdw commented 1 year ago

I feel like this came up, but searching open/closed issues/PRs I only noticed https://github.com/paritytech/json-rpc-interface-spec/pull/61 so maybe I had that in mind.

Anyway, is there any good reason to keep archive_genesisHash? It feels like we should remove it.

tomaka commented 1 year ago

The point of having multiple identical functions with the same prefix is that the prefixed are all self-contained. As mentioned in #61, if you're intending to use the archive function, you don't know whether the node you're connected to will also have chainSpec available.

Removing archive_genesisHash "because we have chainSpec_genesisHash" is not by itself a good motivation.

jsdw commented 1 year ago

Do you ever forsee a time in which a chain would expose archive methods and not chainSpec methods, though?

Removing archive_genesisHash "because we have chainSpec_genesisHash" is not by itself a good motivation.

I don't really agree; I'd see it more as "we shouldn't duplicate methods across prefixes unless there is a good enough reason to".

One argument for keeping it for me is just because archive_hashByHeight I guess allows you to enter 0 as a height to get it anyway, so the logic already needs to exist to make it fetchable already.

Either way, I don't care very much. It just seemed odd when I noticed it.

tomaka commented 1 year ago

Do you ever forsee a time in which a chain would expose archive methods and not chainSpec methods, though?

A software that is simply plugged onto a RocksDB/ParityDB database (not a proper node, not connected to the peer-to-peer network, just reads from the database) wouldn't be able to serve chainSpec.

I don't really agree; I'd see it more as "we shouldn't duplicate methods across prefixes unless there is a good enough reason to".

Why not? "Not duplicating" stuff isn't by itself a good reason. Duplicating code is generally frown upon because you might update one piece of code and forget to update the other, or things like that, but this doesn't apply here.

One argument for keeping it for me is just because archive_hashByHeight I guess allows you to enter 0 as a height to get it anyway, so the logic already needs to exist to make it fetchable already.

You mean a good reason to remove archive_genesisHash? I think the reason why I've put archive_genesisHash in addition to archive_hashByHeight is because it returns a single value instead of an array. But I'm fine with adding a paragraph to hashByHeight that mentions that in the case of 0 it must always return precisely one item.

jsdw commented 1 year ago

A software that is simply plugged onto a RocksDB/ParityDB database (not a proper node, not connected to the peer-to-peer network, just reads from the database) wouldn't be able to serve chainSpec.

Aah, ok for me, this is a good enough justification to me to keep archive_genesisHash; thanks for pointing that case out!

I think the reason why I've put archive_genesisHash in addition to archive_hashByHeight is because it returns a single value instead of an array.

I think I'm fine with this too; I only thought about archive_hashByHeight after I opened this issue, and so on the one hand it also provides the same functionality as archive_genesisHash (with the "only one block treturned at 0" caveat), buton the other hand archive_genesisHash can be seen as a nice, understandable alias to that and it'll always be easy to implement anyway given that archive_hashByHeight needs implementing.

So personally I'm all good here; I'm happy with things staying as they are. Thanks for your explanations!