Add some cfg checks on some types for the Rust sdk

[gRoussac]

Mainly adding sdk = [] as feature in casper types

Add "sdk" cfg feature checks into some types that use io/fs or system calls

Most of the changes are induced by compiling, linting and cargo check on the new sdk feature flag

casper-ecosystem/rustSDK#2

[bpr]

LGTM.

NB: In the future a guide to the changes in any significant PR would be helpful to the reviewer.

[zacshowa]

The only change that I want to see before merging is the change to the location of the change log entry. If you like my idea for the conditional compilation then feel free to use it as I think it could have readability improvements for all similar patters in this PR.

[zacshowa]

It looks like the only code changes in this PR are to the types crate. The description of the work done should be added to the CHANGELOG.md in the types directory.

[gRoussac]

Done thanks

[zacshowa]

I still think that we could aim for 2 different function definitions with something similar to this:

    /// Attempts to write the key bytes to the configured file path.
    ///
    /// This is the actual implementation of the function.
    #[cfg(not(any(feature = "sdk")))]
    #[cfg(any(feature = "std", test))]
    pub fn to_file<P: AsRef<Path>>(&self, file: P) -> Result<(), ErrorExt> {
        write_private_file(file, self.to_pem()?).map_err(ErrorExt::SecretKeySave)
    }

    /// Attempts to write the key bytes to the configured file path.
    ///
    /// This is a no-op for the `sdk` feature.
    #[cfg(any(feature = "std", test, sdk))]
    #[allow(unused_variables)]
    pub fn to_file<P: AsRef<Path>>(&self, file: P) -> Result<(), ErrorExt> {
        Ok(())
    }

I believe it would be functionally equivalent and be easier to read. I'm not married to this though. If you prefer your style, I think its fine.

[gRoussac]

Hi, then #[cfg(any(feature = "std", test, can not be on both. That is already where the confusion starts having two methods.

sdk feature should imo be isolated in those attributes and not mixed with other feature in case we want to change the feature to an additive feature or else remove it fast.

Using 'wasm-strip' version 1.0.31 (git~1.0.31-12-g46e7bc54)
cd types && /opt/rust/.cargo/bin/cargo --locked check --all-targets --no-default-features --features=std
    Checking casper-types v3.0.0 (/media/WINKING/opt2/casper/casper-node/types)
error[E0592]: duplicate definitions with name `to_file`
   --> types/src/crypto/asymmetric_key.rs:253:5
    |
244 |     pub fn to_file<P: AsRef<Path>>(&self, file: P) -> Result<(), ErrorExt> {
    |     ---------------------------------------------------------------------- other definition for `to_file`
...
253 |     pub fn to_file<P: AsRef<Path>>(&self, file: P) -> Result<(), ErrorExt> {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ duplicate definitions for `to_file`

I guess you meant to write. I am not sure.

    #[cfg(any(feature = "sdk"))]
    #[allow(unused_variables)]
        pub fn to_file<P: AsRef<Path>>(&self, file: P) -> Result<(), ErrorExt> {
        Ok(())
    }

this passes cargo check but gives me linting errors.

It is not about a style having two functions with the same code or a config check with the same code in one function. I had much difficulties linting and checking the first style that lead me to use the first style whenever I could, mostly to remove functions and the second style one function with conditional config in it to keep the function for all features.

I do not mind which style I just could not make it pass linting and sometimes checking which lead me to stop trying to multiply show/hide methods but keep them and tweak them inside.

$ make lint-all-features
Using 'wasm-strip' version 1.0.31 (git~1.0.31-12-g46e7bc54)
/opt/rust/.cargo/bin/cargo --locked clippy --all-targets --all-features -- -D warnings
    Checking casper-node v1.5.2 (/media/WINKING/opt2/casper/casper-node/node)
error[E0599]: no method named `to_file` found for struct `std::sync::Arc<casper_types::SecretKey>` in the current scope
   --> node/src/reactor/main_reactor/tests.rs:191:18
    |
191 |                 .to_file(secret_key_path.clone())
    |                  ^^^^^^^ method not found in `std::sync::Arc<casper_types::SecretKey>`

error[E0599]: no method named `to_file` found for struct `std::sync::Arc<casper_types::SecretKey>` in the current scope
   --> node/src/reactor/main_reactor/tests.rs:441:14
    |
441 |             .to_file(secret_key_path.clone())
    |              ^^^^^^^ method not found in `std::sync::Arc<casper_types::SecretKey>`

I really do not mind the style, it's not about style it's about the time I spent to make it pass checking and linting and making various tests and attempts to not affect the rest of features.

Please help me here, checkout the branch and try to change that style and then proceed to checking and linting for all the cases in the makefile and I would be happy to merge. Right now above suggestion does not pass.

check-std-features
check-sdk-features
lint-default-features
lint-sdk-features
lint-all-features

If you succeed to have second style, ie dividing in two functions and making it pass check and lint I really do not mind and I am not married to this style neither and open to modifications.

    #[cfg(any(feature = "std", test))]
    #[allow(unused_variables)]
    pub fn to_file<P: AsRef<Path>>(&self, file: P) -> Result<(), ErrorExt> {
        #[cfg(not(any(feature = "sdk")))]
        {
            write_private_file(file, self.to_pem()?).map_err(ErrorExt::SecretKeySave)
        }
        #[cfg(feature = "sdk")]
        {
            Ok(())
        }
    }

I took this version because it passes linting and checking for all cases.

not a matter of style but a matter of simplicity and keeping it simple.

[gRoussac]

However I committed a bad makefile on the branch so reverting in

c61ce65

[cspramit]

@SaiProServ please follow up on this with the stakeholders and the related story