neo-project / neo-modules

MIT License
60 stars 100 forks source link

RpcServer, RpcClient: strip HF_ prefix from `getversion` response #838

Closed AnnaShaleva closed 10 months ago

AnnaShaleva commented 11 months ago

It's nice to have clear hardfork ame without hardfork prefix so that the resulting hardfork JSON-ized name can be applicable by both C# and NeoGo nodes.

Ref. https://github.com/neo-project/neo-modules/issues/822.

Jim8y commented 11 months ago

weird to make C# aligned to neogo, instead of neogo aligned to C#. If it is not a bug or improvement proposal, i dont have any motivation to stay aligned to neogo.

@shargon Any thought on this?

roman-khimov commented 11 months ago

It's obviously an improvement for the RPC protocol. These prefixes are OK for some code-level constants in a big module, but they're not really interesting for RPC reply that has hardforks section.

Note also, that their introduction into the protocol was accidental, not intentional (as well as inclusion into 3.6.1).

cschuchardt88 commented 11 months ago

you have to remove

https://github.com/neo-project/neo-modules/blob/6ead78007ba6003eb050f7c7bffeb4e2476f348e/tests/Neo.Network.RPC.Tests/RpcTestCases.json#L2495-L2500

for the tests to work again.

Or Change to reflect your changes.

HF_HF_Aspidochelone is your name that you are using. No need to add prefix. the enum name as it already.

Prefix should be removed. and should be like this Aspidochelone.

Its in section hardfork already. so no need for prefix of HF that's redundant

May need to fix #825 as well...

Jim8y commented 11 months ago

Changes to the interface should be very careful, we dont know if someone is already using it or not, and it is not a good experience to change the inteface from time to time. Yes, without HF prefix is much reasonable, but since it has HF and has being in this way for a well, then it become a historical problem.

I wont stand my opinion if @shargon agrees to update, but before that, i dont think it is necessary or should be done in another compatible way without breaking existing interface.

AnnaShaleva commented 10 months ago

As Roman said in https://github.com/neo-project/neo-modules/pull/838#issuecomment-1760143151, the initial idea for hardforks naming was to use "Aspidochelone", "Basilisk" and etc. for user-facing outputs (see the https://github.com/neo-project/neo/pull/2749#discussion_r878971641), and HF_Aspidochelone is just a code-level enum implementation that is expected to stay at the code level. So it would be nice to keep human-readable names in the JSON response output, as we already have them under hardforks section.

AnnaShaleva commented 10 months ago

I prefer to use a method to check if the string startWith, instead of replace always the first 3 chars.

Fixed, see the updated version, please.