-
Notifications
You must be signed in to change notification settings - Fork 46
Add hidden RPCs syncwithvalidationinterfacequeue, mockscheduler and reconsiderblock
#456
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
Conversation
|
Thanks for your contribution! Just a quick warning before I review, this repo is quite strange. If you want more context to any of my review suggestions please ask. |
| () => { | ||
| impl Client { | ||
| pub fn sync_with_validation_interface_queue(&self) -> Result<()> { | ||
| self.call("syncwithvalidationinterfacequeue", &[]) |
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.
For functions that return () we use this pattern
match self.call("preciousblock", &[into_json(hash)?]) {
Ok(serde_json::Value::Null) => Ok(()),
Ok(res) => Err(Error::Returned(res.to_string())),
Err(err) => Err(err.into()),
}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.
EDIT: See comment below before actioning this
integration_test/tests/blockchain.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn blockchain__sync_with_validation_interface_queue__modelled() { |
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.
The __modelled suffix is for RCP methods that have a type defined in types::model.
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.
Also this method is hidden, right? It should be tested in hidden.rs if so.
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.
Oh, my oversight. As I have not defined any types, I'll remove the prefix and move tests to hidden.rs. Pushed a fix. Thanks for the review!
integration_test/tests/blockchain.rs
Outdated
| // Create activity that causes validation callbacks. | ||
| let (_address, _txid) = node.create_mempool_transaction(); | ||
|
|
||
| let _: () = node |
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.
Oh interesting. I like this. It invalidates my previous suggestion, and we could use this pattern throughout the codebase instead of the 3 lines I posted to check for ().
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.
|
Well done, clean. You managed to navigate this odd repository quite well. Thanks! reviewed: 7756c42 |
68f96f2 to
bab4647
Compare
tcharding
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.
ACK bab4647
|
Thanks mate! Just for reference, usually I get folk to squash changes from review into the original patch instead of adding them at the end. Here I elected to just merge because you did a whole bunch of work already, thanks for the effort. |
This PR:
syncwithvalidationinterfacequeueandreconsiderblockin the v17 hidden module, add the client macro and test. Add the reexports for all later versions, as there are no changes in each up to v30.mockschedulerRPC starting from v20 to v30.Part of #333.