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

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

Merged
merged 3 commits into from
Oct 15, 2023

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Oct 12, 2023

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. #822.

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.

Signed-off-by: Anna Shaleva <[email protected]>
@Jim8y
Copy link
Contributor

Jim8y commented Oct 12, 2023

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
Copy link
Contributor

roman-khimov commented Oct 12, 2023

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
Copy link
Member

cschuchardt88 commented Oct 13, 2023

you have to remove

"hardforks": [
{
"name": "HF_Aspidochelone",
"blockheight": 0
}
]

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
Copy link
Contributor

Jim8y commented Oct 13, 2023

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.

Comment on lines 47 to 48
// Strip HF_ prefix.
["name"] = s.Key.ToString().Substring(3),
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

See the #838 (comment) and updated version.

@@ -63,7 +64,7 @@ public static RpcProtocol FromJson(JObject json)
MaxTransactionsPerBlock = (uint)json["maxtransactionsperblock"].AsNumber(),
MemoryPoolMaxTransactions = (int)json["memorypoolmaxtransactions"].AsNumber(),
InitialGasDistribution = (ulong)json["initialgasdistribution"].AsNumber(),
Hardforks = new Dictionary<Hardfork, uint>(((JArray)json["hardforks"]).Select(s => new KeyValuePair<Hardfork, uint>(Enum.Parse<Hardfork>(s["name"].AsString()), (uint)s["blockheight"].AsNumber()))),
Hardforks = new Dictionary<Hardfork, uint>(((JArray)json["hardforks"]).Select(s => new KeyValuePair<Hardfork, uint>(Enum.Parse<Hardfork>($"HF_{s["name"].AsString()}"), (uint)s["blockheight"].AsNumber()))),
Copy link
Member

Choose a reason for hiding this comment

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

you are adding it again. why?

Copy link
Member

Choose a reason for hiding this comment

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

Test fails with System.ArgumentException: Requested value 'HF_HF_Aspidochelone' was not found.

Copy link
Member Author

Choose a reason for hiding this comment

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

I accidentally forgot to commit the test file changes, fixed.

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

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

Strip `HF_` prefix from the hardforks response.
Fix @cschuchardt88's review: neo-project#838 (comment).

Signed-off-by: Anna Shaleva <[email protected]>
AnnaShaleva added a commit to AnnaShaleva/neo-modules that referenced this pull request Oct 13, 2023
@AnnaShaleva
Copy link
Member Author

AnnaShaleva commented Oct 13, 2023

As Roman said in #838 (comment), the initial idea for hardforks naming was to use "Aspidochelone", "Basilisk" and etc. for user-facing outputs (see the neo-project/neo#2749 (comment)), 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
Copy link
Member Author

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.

@Jim8y Jim8y merged commit 12746bf into neo-project:master Oct 15, 2023
3 checks passed
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.

5 participants