-
Notifications
You must be signed in to change notification settings - Fork 22
Implement pruneblock method and test #132
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
base: master
Are you sure you want to change the base?
Conversation
26e01dc
to
371f56c
Compare
371f56c
to
2d05895
Compare
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.
Looks pretty good man. Can you strip out all the unrelated changes then I'll do another review. Keep at it man, you're killing it.
self.call("pruneblockchain", &[target.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.
This is good but can be put it after impl_client_v17__preciousblock
? I've tried to keep all the files using the same order for methods that they appear in the help output i.e., types/src/v17/mod.rs
(or the rpc-api-v17.txt file).
@@ -49,6 +49,7 @@ crate::impl_client_v17__gettxoutproof!(); | |||
crate::impl_client_v17__gettxoutsetinfo!(); | |||
crate::impl_client_v17__preciousblock!(); | |||
crate::impl_client_v17__verifytxoutproof!(); | |||
crate::impl_client_v17__pruneblockchain!(); |
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.
Same thing here, please keep the calls in order.
let json: GetBlockVerboseZero = node.client.get_block_verbose_zero(block_hash).expect("getblock verbose=0"); | ||
let json: GetBlockVerboseZero = | ||
node.client.get_block_verbose_zero(block_hash).expect("getblock verbose=0"); |
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.
These look like unrelated formatting changes. Best not to have these.
use node::vtype::*; // All the version specific types. | ||
use node::vtype::*; // All the version specific types. |
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.
Unrelated change. Looks like we have our editors configured differently. This is the default spacing set by my editor. Neither is right or wrong its just that this is an unrelated whitespace change.
Going by the conversations with @tcharding for PR #116 on implementing one method at a time for easier review: This is the first method implementation
pruneblockchain
which is a specific type that returns a standard type (numeric). Once this is approved, I’ll proceed with the remaining ones.