Add sdk feature into feat 2.0 #106
Conversation
# Conflicts: # Cargo.toml # lib/cli.rs # lib/cli/deploy.rs
…tem/casper-client-rs into rustSDK # Conflicts: # Cargo.toml # lib/cli.rs # lib/cli/parse.rs
There was a problem hiding this comment.
LGTM, but please wait for @Fraser999 and @zacshowa to review before merging. There's a lot of changes and I think we should tread carefully here.
| @@ -123,3 +98,38 @@ pub fn new_transfer( | |||
| .map_err(crate::Error::from)?; | |||
| Ok(deploy) | |||
| } | |||
|
|
|||
| fn get_maybe_secret_key( | |||
There was a problem hiding this comment.
The change to this function is interesting.
Perhaps we could look to overload the function by conditionally compiling 1 of 2 functions of the same name behind feature flags as opposed to modifying the body of one function for readability.
There was a problem hiding this comment.
Well mostly this code was repeated two times in the file and we already discussed this condition
let maybe_secret_key = if allow_unsigned_deploy && deploy_params.secret_key.is_empty() {
None
} else if deploy_params.secret_key.is_empty() && !allow_unsigned_deploy {
is imo more idiomatic in if A / else if B / else. It's quite complicate to have cargo check and cargo clippy accepting overloads, I tried to have 1/2 methods with both config accordingly and that never passed cargo check/clippy. Most of times the only accepted way is to simply have #[cfg] working like if statements. Otherwise most of the times a function will be missing on compilation if you have like --features-all
There was a problem hiding this comment.
The structure of the if else is fine, It's functionally equivalent and should work fine.
I do think that if you like the style suggestion I put on the sister PR against the node I would love to see the same style applied to the feature flag here. If you don't want to go with that style, consider the PR approved.
There was a problem hiding this comment.
Casper client do not seem to have cargo clippy so I can duplicate this method in the client without side effects apparently.
Please check that commit 4855e7b thanks
| @@ -123,3 +98,38 @@ pub fn new_transfer( | |||
| .map_err(crate::Error::from)?; | |||
| Ok(deploy) | |||
| } | |||
|
|
|||
| fn get_maybe_secret_key( | |||
There was a problem hiding this comment.
The structure of the if else is fine, It's functionally equivalent and should work fine.
I do think that if you like the style suggestion I put on the sister PR against the node I would love to see the same style applied to the feature flag here. If you don't want to go with that style, consider the PR approved.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Change import JsonArg
Right now this feature only build with --lib
Mainly adds some
#[cfg(feature = "sdk")]where it is possible or not to use io/fs, or prevent sending to std withjson_pretty_printand returning deploysChanges the way secret key is loaded in
with_payment_and_session()Publicizes some methods outside the crate
Adds
session_bytestosession_executable_deploy_item()and addscheck_no_conflicting_arg_types()(might require additional unit tests)Adds
with_bytes()toSessionStrParamsAdds some
Cloneattributes to some structsRemoves rt-multi-thread,
The default runtime flavor is 'multi_thread', but the 'rt-multi-thread' feature is disabled.switching (flavor = "current_thread") to tokio mainI could not make it compile without this change in cargo.toml related to casper types
getrandom = { version = "0.2.10", features = ["js"] }getrandom probably needs to be replaced by something else.Removing
[patch.crates-io]as I believe this bug was fixed and this is not needed anymore.casper-ecosystem/rustSDK#4
casper-ecosystem/rustSDK#5