Problem: (CRO-494) not possible to create unjail transaction in client-cli#556
Conversation
|
fixed viewkey |
Codecov Report
@@ Coverage Diff @@
## master #556 +/- ##
==========================================
- Coverage 67.58% 67.54% -0.04%
==========================================
Files 124 124
Lines 14320 14332 +12
==========================================
+ Hits 9678 9681 +3
- Misses 4642 4651 +9
|
tomtau
left a comment
There was a problem hiding this comment.
looks ok, but can be simplified:
-
feature guards in cli don't seem to help with testing cli-related functionality (as the actual functionality is skipped) -- it'd be preferable not to have them... a separate PR could improve testing of cli by replacing stdin/stdout in tests with custom streams
-
HD wallet syncing is a different problem, so that fix should go to a different PR
client-cli/src/main.rs
Outdated
| //! CLI for interacting with Crypto.com Chain | ||
| mod command; | ||
|
|
||
| #[allow(unused_imports)] |
client-cli/src/main.rs
Outdated
| #[cfg(not(test))] | ||
| pub(crate) fn ask_passphrase(message: Option<&str>) -> Result<SecUtf8> { | ||
| ask(message.unwrap_or("Enter passphrase: ")); | ||
| password() | ||
| .map(Into::into) | ||
| .chain(|| (ErrorKind::IoError, "Unable to read password")) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub(crate) fn ask_passphrase(_message: Option<&str>) -> Result<SecUtf8> { | ||
| Ok(SecUtf8::from("123456")) | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not sure if this helps with testing, as it'll skip the actual code to be tested and have a replacement -- most things in client are tested in isolation.
For cli testing, it's a bit tricky, but ideally it'd be done with passing in custom streams instead of using stdin/stdout
There was a problem hiding this comment.
could you make entering value via stream as separate issue?
so we can unit-test for those functions.
| .expect("get staked address"), | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
again not sure if this helps much with testing -- left a more detailed comment below
There was a problem hiding this comment.
it's for coverage failure.
i'll find other way
There was a problem hiding this comment.
coverage is just for reference -- no need to "hack" it in this way; better to do proper tests
| self.key_service | ||
| .add_keypair(&private_key, &public_key, passphrase)?; | ||
|
|
||
| self.wallet_service.create(name, passphrase, public_key) |
There was a problem hiding this comment.
this appears to solve a different problem than stated in this PR -- I guess it's "Problem: syncing doesn't work with HD wallets", please open a separate PR for it
There was a problem hiding this comment.
i'll make separate PR
| name, | ||
| &TransactionType::Unjail, | ||
| ) | ||
| .is_ok()); |
There was a problem hiding this comment.
This formatting seems a bit odd to me.
There was a problem hiding this comment.
I think you can use .is_err() for !xxx.is_ok()
There was a problem hiding this comment.
Forget about the format, actually it is normal
There was a problem hiding this comment.
because it's for coverage failure, i'll remove this for now.
will test via stream.
| } | ||
|
|
||
| #[test] | ||
| fn check_unjail_tx() { |
There was a problem hiding this comment.
Perhaps to give more details about this test to make the test more self-explanatory.
|
i'll fix |
0ff5604 to
6c358dc
Compare
…t-cli Solution: call unjail in cli increase coverage fix sync fix save public, private key remove un-necessary unit-test restore wallet code reformat
6c358dc to
c639b64
Compare
556: Problem: (CRO-494) not possible to create unjail transaction in client-cli r=tomtau a=leejw51 Solution: call unjail in cli increase coverage fix sync fix save public, private key 561: Problem: new drone pipeline not authorized r=tomtau a=tomtau Solution: updated the signature Co-authored-by: Jongwhan Lee <jonghwan@crypto.com> Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com>
Build failed (retrying...) |
|
bors r+ |
538: Problem:(CRO-521) unbonded from custom time is ignored in genesis initconfig r=tomtau a=linfeng-crypto Solution: change the parameters of `new_init`: change `genesis_time` from `Timespec` into `Option<Timespec>`, remove the `bool` type parameter `bonded`, add a `&StakedStateDestination` type parameter. 556: Problem: (CRO-494) not possible to create unjail transaction in client-cli r=tomtau a=leejw51 Solution: call unjail in cli increase coverage fix sync fix save public, private key 558: Problem: Inefficient binary encoding of Merkle tree path (CRO-149) r=tomtau a=yihuang I've refactored the ``Path`` representation with ``Vec``, which makes it both more correct and more compact serialization (Old representation allows some illegal path state). With the current implementation, the gain of further optimizing the serialization with ``BitVec`` is small, I guess it doesn't worth the trouble anymore. Co-authored-by: linfeng <linfeng@crypto.com> Co-authored-by: Jongwhan Lee <jonghwan@crypto.com> Co-authored-by: yihuang <huang@crypto.com>
Canceled (will resume) |
538: Problem:(CRO-521) unbonded from custom time is ignored in genesis initconfig r=tomtau a=linfeng-crypto Solution: change the parameters of `new_init`: change `genesis_time` from `Timespec` into `Option<Timespec>`, remove the `bool` type parameter `bonded`, add a `&StakedStateDestination` type parameter. 556: Problem: (CRO-494) not possible to create unjail transaction in client-cli r=tomtau a=leejw51 Solution: call unjail in cli increase coverage fix sync fix save public, private key Co-authored-by: linfeng <linfeng@crypto.com> Co-authored-by: Jongwhan Lee <jonghwan@crypto.com>
Build failed (retrying...) |
Solution: call unjail in cli
increase coverage
fix sync
fix save public, private key