refactor: use macros to register the subscription APIs#5782
refactor: use macros to register the subscription APIs#5782akaladarshi merged 4 commits intoelmattic/eth-subscribefrom
Conversation
6163337 to
4688372
Compare
4688372 to
c33fd45
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| // TODO(akaladarshi): https://github.com/ChainSafe/forest/pull/5782 | ||
| return Err(SubscriptionError::from( | ||
| jsonrpsee::types::ErrorObjectOwned::owned( | ||
| jsonrpsee::types::error::METHOD_NOT_FOUND_CODE, | ||
| "pendingTransactions subscription not yet implemented", | ||
| None::<()>, | ||
| ), | ||
| )); |
There was a problem hiding this comment.
Does it reference the right issue? Seems to be referencing the same PR.
There was a problem hiding this comment.
Removing the reference from here, since there is no specific issue for it.
| ) -> jsonrpsee::core::SubscriptionResult; | ||
| } | ||
|
|
||
| // Keep the existing types but make them more structured |
There was a problem hiding this comment.
leftover from ai?
| // Keep the existing types but make them more structured |
LesnyRumcajs
left a comment
There was a problem hiding this comment.
Are the subscription methods still present in the openrpc spec after these changes?
| let eth_pubsub = EthPubSub::new(state.clone()); | ||
| module | ||
| .merge(eth_pubsub.into_rpc()) | ||
| .expect("Could not merge eth pubsub module"); |
There was a problem hiding this comment.
Remove expect here and propagate the error, like for the fil pubsub module.
|
It's not clear based on your branch name, if this PR brings support of |
Summary of changes
Changes introduced in this pull request:
jsonrpseemacros to register the eth subscribe and unsubscribe API'sReference issue to close (if applicable)
Closes
Other information and links
Change checklist