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

neorpc: change peer port to int type (fixes #2910) #2914

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

ixje
Copy link
Contributor

@ixje ixje commented Feb 14, 2023

Problem

#2910 type mismatch

Solution

Change type from string to int.

@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Merging #2914 (f479b2b) into master (ce7658e) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2914      +/-   ##
==========================================
+ Coverage   85.05%   85.12%   +0.06%     
==========================================
  Files         329      329              
  Lines       42549    42580      +31     
==========================================
+ Hits        36191    36245      +54     
+ Misses       4894     4865      -29     
- Partials     1464     1470       +6     
Impacted Files Coverage Δ
pkg/neorpc/result/peers.go 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

This technically breaks compatibility for us, so

  1. It's for 0.102.0 (but we're very likely to skip 0.101.1 anyway)
  2. There is a question about old servers support. On one hand it seems to be a rarely used API, on the other we don't know for sure. We can add some compatibility glue for it like we did in previous cases (9862b40 or c465b18). @AnnaShaleva, what do you think?

@ixje
Copy link
Contributor Author

ixje commented Feb 14, 2023

This technically breaks compatibility for us, so

compatibility with what other part? If we're talking NeoNodes then arguably it was not compatible to begin with. As far as I looked in the C# code this type has not changed in the last 4 years.

@roman-khimov
Copy link
Member

Compatibility with NeoGo nodes. Old clients couldn't connect to new servers, that we can explain (upgrade!), but new clients won't be able to interact with old nodes, that's more problematic. Custom UnmarshalJSON() handling both strings and ints is needed for some transition period (see ROADMAP.md also).

@AnnaShaleva
Copy link
Member

AnnaShaleva commented Feb 15, 2023

As for me, I extremely like the way how it is done in #2448. It looks ugly in the code, yes, but at the same time it's important for us to support the compatibility with the old nodes, otherwise new client's code just won't work with them. We need to add a custom umarshaler as @roman-khimov wrote and add a note for its deprecation/removal in future releases for better user experience.

@ixje
Copy link
Contributor Author

ixje commented Feb 15, 2023

I did my approach before seeing Anna's link to #2448. Let me know if this works, otherwise I'll try to digest that PR and adjust where necessary.

@roman-khimov
Copy link
Member

#2448 is pretty complex because it was important to exactly check version there. Here a simple try this/try that approach should be OK.

@ixje
Copy link
Contributor Author

ixje commented Feb 15, 2023

Is there some doc describing all tools and rules I need to configure my IDE with? I'm willing to try and solve my own issue when time allows, but things like linting and style errors not reported by my IDE get on my nerves

@roman-khimov
Copy link
Member

make lint, make fmt can help a bit (+goimports), most likely you can automatically run go fmt as pre-save action.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

An entry in ROADMAP.md is also needed (we usually schedule things like this 6 months away from the release the change appears in).

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Could you, please, squash the commits?

@ixje
Copy link
Contributor Author

ixje commented Feb 16, 2023

Could you, please, squash the commits?

Will do, I was waiting to see if there was any feedback left before doing so.

@ixje ixje requested review from AnnaShaleva and removed request for fyrchik February 20, 2023 18:12
@ixje
Copy link
Contributor Author

ixje commented Feb 20, 2023

fwiw; the lint failures are not in code that I touched

@roman-khimov
Copy link
Member

@ixje, yeah I've seen those in another PR as well, looks like a linter upgrade (to be fixed of course).

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Looks OK, we'll merge it when we're closer to 0.102.0.

@roman-khimov
Copy link
Member

@ixje, please rebase it to the current master, it's time to merge, but there is some minor conflict (likely caused by #2957).

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

A nice one, thank you for your contribution!

@roman-khimov roman-khimov merged commit 601c26b into nspcc-dev:master Apr 7, 2023
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.

3 participants