RpcServer, RpcClient: strip HF_ prefix from `getversion` response by AnnaShaleva · Pull Request #838 · neo-project/neo-modules

AnnaShaleva · 2023-10-12T15:49:37Z

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 <shaleva.ann@nspcc.ru>

Jim8y · 2023-10-12T16:59:53Z

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 · 2023-10-12T18:22:03Z

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 · 2023-10-13T02:39:36Z

you have to remove

neo-modules/tests/Neo.Network.RPC.Tests/RpcTestCases.json

Lines 2495 to 2500 in 6ead780

    
           "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 · 2023-10-13T03:07:39Z

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.

cschuchardt88 · 2023-10-13T03:13:17Z

src/RpcClient/Models/RpcVersion.cs

+                    // Strip HF_ prefix.
+                    ["name"] = s.Key.ToString().Substring(3),


See the #838 (comment) and updated version.

cschuchardt88 · 2023-10-13T03:13:34Z

src/RpcClient/Models/RpcVersion.cs

                    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()))),


you are adding it again. why?

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

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

shargon

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

@cschuchardt88

Strip `HF_` prefix from the hardforks response. Fix @cschuchardt88's review: neo-project#838 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

@shargon

Fix @shargon's review: neo-project#838 (review). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

@shargon

Fix @shargon's review: neo-project#838 (review). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva · 2023-10-13T10:22:02Z

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 · 2023-10-13T10:22:43Z

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.

AnnaShaleva mentioned this pull request Oct 12, 2023

Neo v3.6.2 plan neo-project/neo#2914

Closed

RpcServer, RpcClient: strip HF_ prefix from getversion response

ef9dff0

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 <shaleva.ann@nspcc.ru>

AnnaShaleva force-pushed the adj-hf-resp branch from 5d82e63 to ef9dff0 Compare October 12, 2023 15:52

cschuchardt88 reviewed Oct 13, 2023

View reviewed changes

shargon reviewed Oct 13, 2023

View reviewed changes

tests: adjust getversion test response

219c1a8

Strip `HF_` prefix from the hardforks response. Fix @cschuchardt88's review: neo-project#838 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva added a commit to AnnaShaleva/neo-modules that referenced this pull request Oct 13, 2023

Check HF_ prefix before cutting it from hardforks JSON representation

6468dc7

Fix @shargon's review: neo-project#838 (review). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

Check HF_ prefix before cutting/adding it to JSON-ized hardforks

2015178

Fix @shargon's review: neo-project#838 (review). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva force-pushed the adj-hf-resp branch from 6468dc7 to 2015178 Compare October 13, 2023 10:07

AnnaShaleva requested review from cschuchardt88 and shargon October 13, 2023 10:22

Jim8y approved these changes Oct 13, 2023

View reviewed changes

cschuchardt88 approved these changes Oct 13, 2023

View reviewed changes

AnnaShaleva mentioned this pull request Oct 13, 2023

rpcsrv: add hardforks to getversion response nspcc-dev/neo-go#3160

Merged

Jim8y merged commit 12746bf into neo-project:master Oct 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

RpcServer, RpcClient: strip HF_ prefix from `getversion` response#838
Jim8y merged 3 commits intoneo-project:masterfrom
AnnaShaleva:adj-hf-resp

AnnaShaleva commented Oct 12, 2023 •

edited

Loading

Uh oh!

Jim8y commented Oct 12, 2023 •

edited

Loading

Uh oh!

roman-khimov commented Oct 12, 2023 •

edited

Loading

Uh oh!

cschuchardt88 commented Oct 13, 2023 •

edited

Loading

Uh oh!

Jim8y commented Oct 13, 2023 •

edited

Loading

Uh oh!

cschuchardt88 Oct 13, 2023

Uh oh!

AnnaShaleva Oct 13, 2023

Uh oh!

cschuchardt88 Oct 13, 2023

Uh oh!

cschuchardt88 Oct 13, 2023

Uh oh!

AnnaShaleva Oct 13, 2023

Uh oh!

shargon left a comment •

edited

Loading

Uh oh!

AnnaShaleva commented Oct 13, 2023 •

edited

Loading

Uh oh!

AnnaShaleva commented Oct 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		// Strip HF_ prefix.
		["name"] = s.Key.ToString().Substring(3),

Conversation

AnnaShaleva commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jim8y commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roman-khimov commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cschuchardt88 commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jim8y commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cschuchardt88 Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

shargon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnnaShaleva commented Oct 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AnnaShaleva commented Oct 12, 2023 •

edited

Loading

Jim8y commented Oct 12, 2023 •

edited

Loading

roman-khimov commented Oct 12, 2023 •

edited

Loading

cschuchardt88 commented Oct 13, 2023 •

edited

Loading

Jim8y commented Oct 13, 2023 •

edited

Loading

shargon left a comment •

edited

Loading

AnnaShaleva commented Oct 13, 2023 •

edited

Loading