-
Notifications
You must be signed in to change notification settings - Fork 254
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: refactor signing in order to more easily be able to dryrun #547
Conversation
User @savudani8, please sign the CLA here. |
Co-authored-by: Sander Bosma <sanderbosma@gmail.com> Co-authored-by: Daniel Savu <savudani04@yahoo.ro>
Looks like the clippy error is unrelated to this PR. Maybe it has to do with the used toolchain? |
Yup, I suspect owing to a new version of rust and such being released yesterday. I'll look into it! |
A fix for the clippy error is on it's way (#548) I need to stare at the code a little more but offhand it looks good to me; thankyou for the PR! |
If you merge up from master, the clippy error should go away now! |
subxt/src/rpc.rs
Outdated
/// Returns `Ok` with an [`ApplyExtrinsicResult`], which is the result of applying of an extrinsic. | ||
pub async fn dry_run<'client, Y, X, E, Evs>( | ||
&self, | ||
signed_submittable_extrinsic: &SignedSubmittableExtrinsic<'client, Y, X, E, Evs>, |
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.
Since we only need the bytes, how about this just takes in a Vec<u8>
, so we don't need the various lifetimes and generics (I'd go with a &[u8]
if possible but that may be trickier)?
The SignedSubmittableExtrinsic
could then return a &[u8]
(give the users the option to clone or not) from the encoded()
function to pass in here.
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.
In fact, I think if we go with &[u8]
here, we can use hex::encode
to turn that into hex for the RPC params.
subxt/src/client.rs
Outdated
pub fn encoded(&self) -> Encoded { | ||
self.encoded.clone() | ||
} |
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.
As suggested in the other comment, I reckon this can return a &[u8]
, giving users the option to clone or not as needed. I'd add some docs too; something like:
/// Returns the SCALE encoded extrinsic bytes.
Looking great; a couple of small suggestions but otherwise I am really liking this separation; thank you for the contribution! |
I haven't followed this pr too closely but in the initial commit it was possible to do
which in my opinion is a little bit less elegant. @jsdw would you be opposed to adding |
I'd be perfectly up for having a dry_run function on |
For some reason the tests are not finishing execution on my machine. I followed the GitHub workflow, including the substrate binary here, but I get this:
|
One thing to do to fix the CI tests is to prepend "0x" to the hex string generated by As for local tests not working, my only guess is that the substrate binary isn't on your PATH or something like that, so all of the tests are waiting for it to run (I assume no integration tests complete). |
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 good to me!
Co-authored-by: Sander Bosma sanderbosma@gmail.com
Co-authored-by: Daniel Savu savudani04@yahoo.ro
create_signed
is refactored, adding support fordry_run
ning an extrinsic before sending it.