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

[Remove] obsolete WsServer #3582

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

cschuchardt88
Copy link
Member

Change Log

  • Removed obsolete for NodeCapabilityType.
  • Removed code for NodeCapabilityType.WsServer
  • Fixed tests

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Size_Get
  • DeserializeAndSerialize

Test Configuration:

  • Unit Tests

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

vncoelho
vncoelho previously approved these changes Nov 16, 2024
Jim8y
Jim8y previously approved these changes Nov 16, 2024
@shargon shargon dismissed stale reviews from Jim8y and vncoelho via 10301ce November 16, 2024 10:39
/// Indicates that the node is listening on a WebSocket port.
/// </summary>
[Obsolete]
WsServer = 0x02,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's correct to remove it like this. Technically it existed in the P2P protocol, so at least the number should be kept as reserved.

Copy link
Member

Choose a reason for hiding this comment

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

ObsoleteWsServer

Copy link
Member Author

Choose a reason for hiding this comment

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

we can always repurpose it in the future. Reserving it says to me that it still WsServer or active.

Copy link
Contributor

@Jim8y Jim8y Nov 17, 2024

Choose a reason for hiding this comment

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

I do not agree, either we should implement it or w should remove it, having somthing in the core that is not implemented (and is not documented) is not a good practice, might exist in the past by mistake, but should be fixed, there is no hardfork for this, should not bother.

I have to say that there are too many things in the neo core that is not documented, not commented, not explained, not in proposal, the only way we learn it is by reading the code, but this wss is even a trap of implementation, it exists, but not work, as chris said "Reserving it says to me that it still WsServer or active.". only very few of us know how they works, this is by no means a good practice.

@NGDAdmin NGDAdmin merged commit 849b2c8 into neo-project:master Nov 18, 2024
7 checks passed
cschuchardt88 added a commit that referenced this pull request Nov 30, 2024
shargon added a commit that referenced this pull request Nov 30, 2024
This reverts commit 849b2c8.

Co-authored-by: Shargon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants