-
Notifications
You must be signed in to change notification settings - Fork 107
feat: updated logic to now consider mentions while filtering #403
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
feat: updated logic to now consider mentions while filtering #403
Conversation
|
Hi, I am excited to contribute to Surfpool. This is my first Rust PR, so I have focused on getting the core logic right before writing tests. I'd especially appreciate feedback on:
I am ready to quickly iterate based on your feedback. |
|
This is awesome, @NiravJoshi33! The logic looks great, with some clean rust patterns! Well done! I'm realizing we don't actually have tests for WS RPC methods yet. Would you want to tackle the first ones? Our |
|
Ah, actually I couldn't push to your fork! You can enable me pushing, or this is the fn I added to the bottom of #[tokio::test(flavor = "multi_thread")]
async fn test_ws_scaffold() {
use std::sync::RwLock as StdRwLock;
let (svm_instance, _simnet_events_rx, _geyser_events_rx) = SurfnetSvm::new();
let svm_locker = SurfnetSvmLocker::new(svm_instance);
let runtime = tokio::runtime::Builder::new_multi_thread()
.enable_all()
.build()
.expect("Failed to build Tokio runtime");
let tokio_handle = runtime.handle();
let uid = std::sync::atomic::AtomicUsize::new(0);
let ws_server = crate::rpc::ws::SurfpoolWsRpc {
uid,
signature_subscription_map: Arc::new(StdRwLock::new(HashMap::new())),
account_subscription_map: Arc::new(StdRwLock::new(HashMap::new())),
slot_subscription_map: Arc::new(StdRwLock::new(HashMap::new())),
logs_subscription_map: Arc::new(StdRwLock::new(HashMap::new())),
tokio_handle: tokio_handle.clone(),
};
// subscribe to a method you want to test
// use the svm_locker to write to the vm and/or trigger events
} |
|
Thanks so much @MicaiahReid! Really appreciate the feedback and the test scaffold. 🙏 I'll check the test scaffold and study existing patterns, and start working on the tests. Thanks for the opportunity to work on something foundational. |
|
Hey @NiravJoshi33, any updates on those tests? |
|
Hi @MicaiahReid, I am working on them but got little busy with something else. I'll make some more progress with them and share it in this pr for your review within 1-2 days. |
|
I have added comprehensive tests for the ws methods: Total 22 tests for the implemented 4 methods.
These tests validate the internal subscription mechanisms rather than full E2E websocket connections. This approach is consistent with existing integration tests and provides comprehensive coverage of the subscription notification logic. |
MicaiahReid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NiravJoshi33, this is incredible! Thank you for adding all of these tests!
I added some minor changes, but I'm good to approve. My changes were:
- use the
test_caseimport to simplify some repeat logic on the signature subscription tests - for the logs subscriptions, I added a
mentionsfilter that should not be yielding log results to assert that the mentions will properly filter logs out (your test confirmed that mentioned accounts would be in the logs, but not the other way around as well) - cleaned up imports
Fixes Issue #242
Implements the missing
mentionsfield filtering in thelogsSubscribe/logsUnsubscribeWebSocket functionality.notify_logs_subscribersmethodAll,AllWithVotes, andMentions(Vec<String>)filterscontinuewhen commitment level doesn't matchNext Steps: Tests covering different behaviors of this feature.
Feedback is very much welcomed. Happy to iterate on the approach before investing time in tests.