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.

Sign up for GitHub

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

Jump to bottom

rpc: subscription filters validity check #3258

Merged
merged 2 commits into from
Dec 28, 2023
Merged

rpc: subscription filters validity check #3258

merged 2 commits into from
Dec 28, 2023

Conversation

AliceInHunterland
Copy link
Contributor

@AliceInHunterland AliceInHunterland commented Dec 9, 2023 •
edited
Loading

Change BlockFilter Primary field from int to byte. Additional check of filters parameters added for filter validation.

Closes: #3241

@AliceInHunterland AliceInHunterland force-pushed the rpc_validation branch 2 times, most recently from d5f4325 to 04c5f2c Compare December 13, 2023 06:57
@AliceInHunterland AliceInHunterland marked this pull request as ready for review December 13, 2023 06:58
AnnaShaleva
AnnaShaleva reviewed Dec 13, 2023
View reviewed changes
docs/notifications.md Outdated Show resolved Hide resolved
docs/notifications.md Outdated Show resolved Hide resolved
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/rpcclient/wsclient.go Show resolved Hide resolved
pkg/services/rpcsrv/server.go Outdated Show resolved Hide resolved
AnnaShaleva
AnnaShaleva reviewed Dec 13, 2023
View reviewed changes
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
AnnaShaleva
AnnaShaleva reviewed Dec 14, 2023
View reviewed changes
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/neorpc/filters.go Show resolved Hide resolved
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/neorpc/rpcevent/filter.go Outdated Show resolved Hide resolved
pkg/rpcclient/wsclient.go Outdated Show resolved Hide resolved
pkg/rpcclient/wsclient.go Show resolved Hide resolved
pkg/rpcclient/wsclient.go Outdated Show resolved Hide resolved
pkg/services/rpcsrv/server.go Outdated Show resolved Hide resolved
pkg/services/rpcsrv/server.go Outdated Show resolved Hide resolved
AnnaShaleva
AnnaShaleva reviewed Dec 14, 2023
View reviewed changes
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.

Please, remove "TODO" from the PR description.

@codecov Codecov
Copy link

codecov bot commented Dec 14, 2023 •
edited
Loading

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (468d98d) 85.21% compared to head (802006b) 85.25%.
Report is 6 commits behind head on master.

❗ Current head 802006b differs from pull request most recent head cc06674. Consider uploading reports for the commit cc06674 to get more accurate results

Files Patch % Lines
pkg/rpcclient/wsclient.go 46.42% 27 Missing and 3 partials ⚠️
pkg/core/blockchain.go 94.11% 1 Missing ⚠️
pkg/services/rpcsrv/server.go 95.83% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3258      +/-   ##
==========================================
+ Coverage   85.21%   85.25%   +0.04%     
==========================================
  Files         327      327              
  Lines       44248    44267      +19     
==========================================
+ Hits        37705    37740      +35     
+ Misses       5047     5033      -14     
+ Partials     1496     1494       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AnnaShaleva
AnnaShaleva reviewed Dec 15, 2023
View reviewed changes
pkg/neorpc/filters.go Show resolved Hide resolved
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/services/rpcsrv/server.go Outdated Show resolved Hide resolved
pkg/rpcclient/wsclient.go Show resolved Hide resolved
AnnaShaleva
AnnaShaleva reviewed Dec 15, 2023
View reviewed changes
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please, adjust the PR description, PR includes not only int -> byte filter field change.

AnnaShaleva
AnnaShaleva reviewed Dec 20, 2023
View reviewed changes
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.

Otherwise LGTM.

pkg/neorpc/filters.go Outdated Show resolved Hide resolved
pkg/rpcclient/wsclient_test.go Outdated Show resolved Hide resolved
pkg/rpcclient/wsclient_test.go Outdated Show resolved Hide resolved
pkg/services/rpcsrv/subscription_test.go Outdated Show resolved Hide resolved
pkg/services/rpcsrv/subscription_test.go Outdated Show resolved Hide resolved
pkg/services/rpcsrv/subscription_test.go Outdated Show resolved Hide resolved
pkg/services/rpcsrv/subscription_test.go Outdated Show resolved Hide resolved
pkg/services/rpcsrv/subscription_test.go Outdated Show resolved Hide resolved
pkg/services/rpcsrv/subscription_test.go Outdated Show resolved Hide resolved
pkg/services/rpcsrv/subscription_test.go Outdated Show resolved Hide resolved
roman-khimov
roman-khimov reviewed Dec 21, 2023
View reviewed changes
docs/notifications.md Outdated Show resolved Hide resolved
pkg/neorpc/filters.go Show resolved Hide resolved
pkg/neorpc/filters.go Show resolved Hide resolved
@AliceInHunterland AliceInHunterland force-pushed the rpc_validation branch 2 times, most recently from c0c05d1 to 3e3bd5f Compare December 22, 2023 12:09
AnnaShaleva
AnnaShaleva reviewed Dec 25, 2023
View reviewed changes
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.

After all changes ensure that the Linter job passes.

docs/notifications.md Outdated Show resolved Hide resolved
docs/notifications.md Outdated Show resolved Hide resolved
pkg/rpcclient/wsclient.go Show resolved Hide resolved
roman-khimov
roman-khimov reviewed Dec 25, 2023
View reviewed changes
docs/notifications.md Outdated Show resolved Hide resolved
docs/notifications.md Outdated Show resolved Hide resolved
@AliceInHunterland
rpc: change the type of BlockFilter.Primary from int to byte
275e814 
BlockFilter has PrinaryIndex of int type while block.Block structure
itself has PrimaryIndex of byte. It's needed to prevent changing
filters field type and all associated subscriptions logic on
server side.

Refs #3241.

Signed-off-by: Ekaterina Pavlova <[email protected]>
AnnaShaleva
AnnaShaleva reviewed Dec 26, 2023
View reviewed changes
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.

Otherwise LGTM.

docs/notifications.md Outdated Show resolved Hide resolved
AnnaShaleva
AnnaShaleva reviewed Dec 26, 2023
View reviewed changes
pkg/neorpc/filters.go Outdated Show resolved Hide resolved
@AliceInHunterland
neorpc: add WS notification filter IsValid functionality
cc06674 
Additional check of filters parameters added for filter validation.

Closes #3241.

Signed-off-by: Ekaterina Pavlova <[email protected]>
@AnnaShaleva AnnaShaleva merged commit 3176f72 into master Dec 28, 2023
15 of 17 checks passed
@AnnaShaleva AnnaShaleva deleted the rpc_validation branch December 28, 2023 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnnaShaleva AnnaShaleva AnnaShaleva approved these changes

@roman-khimov roman-khimov Awaiting requested review from roman-khimov roman-khimov is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Audit RPC notification subsystem filters against target structure types
3 participants
@AliceInHunterland @roman-khimov @AnnaShaleva