Base CLI parsing #17

koxu1996 · 2024-02-05T11:36:09Z

CLI interface

Implemented interface with three key transaction commands (Deposit, Transfer, Withdraw) and integrated Casper's cryptography functionalities.

Fixes #14.

Deposit:

$ ./kairos-cli deposit \
  --amount 123 \
  --private-key ~/wallet/secret_key.pem

Transfer:

$ ./kairos-cli transfer \
  --recipient 01a26419... \
  --amount 123 \
  --private-key ~/wallet/secret_key.pem

Withdraw:

$ ./kairos-cli withdraw \
  --amount 123 \
  --private-key ~/wallet/secret_key.pem

Outputs

amount: u64 - no need to handle u512 at this point
signer: CasperSigner (created from CasperPrivateKey)
recipient: CasperPublicKey

Notably, signer allows generating signature of arbitrary byte data. It will be used to sign transactions sent to Kairos server.

Error handling

Invalid transaction amount, for example 123a:

error: invalid value '123a' for '--amount <NUM_MOTES>': invalid digit found in string

Invalid recipient foobar, when making transfer:

error: invalid value 'foobar' for '--recipient <PUBLIC_KEY>': failed to parse hex string: Invalid character 'o' at position 1

Invalid private key path, when content cannot be read:

cryptography error: failed to parse private key: secret key load failed: could not read '/non/exisiting/file/path': No such file or directory (os error 2)

Invalid private key content:

cryptography error: failed to parse private key: pem error: malformedframing

Tests

Both 3 happy paths and error cases are tested in cli_tests.rs.

kairos-cli/bin/commands/mod.rs

kairos-cli/bin/main.rs

kairos-cli/tests/cli_tests.rs

Avi-D-coder

lgtm

koxu1996 · 2024-02-06T22:49:51Z

Solved issue with failing Nix tests: ade64d1 💪. Now it works:

marijanp

I would like to see code that leverages the type system and is more more declarative. That leaves us with code that is easier to follow and extend. Moreover, the compiler supports us to catch missing/ unhandled commands.

kairos-cli/bin/commands/deposit.rs

kairos-cli/bin/main.rs

kairos-cli/bin/crypto/error.rs

kairos-cli/bin/crypto/private_key.rs

kairos-cli/bin/crypto/public_key.rs

kairos-cli/bin/error.rs

kairos-cli/bin/main.rs

kairos-cli/tests/cli_tests.rs

kairos-cli/Cargo.toml

koxu1996 · 2024-02-12T18:02:14Z

PR and description updated - ready for re-review 👀.

kairos-cli/bin/commands/transfer.rs

kairos-cli/bin/crypto/private_key.rs

kairos-cli/bin/crypto/signer.rs

kairos-cli/bin/crypto/error.rs

kairos-cli/bin/crypto/private_key.rs

marijanp

Great, I think we are almost there. One last remark. I've put the main.rs into build to create a separation between library and binary. If you could put all the modules except main.rs into a src directory that would be perfect.

kairos-cli/bin/commands/transfer.rs

kairos-cli/bin/commands/mod.rs

kairos-cli/bin/crypto/error.rs

kairos-cli/bin/commands/transfer.rs

kairos-cli/bin/crypto/private_key.rs

kairos-cli/bin/crypto/public_key.rs

kairos-cli/bin/crypto/signer.rs

kairos-cli/bin/common/args.rs

kairos-cli/bin/error.rs

koxu1996 · 2024-02-13T17:13:14Z

@marijanp

Great, I think we are almost there. One last remark. I've put the main.rs into build to create a separation between library and binary. If you could put all the modules except main.rs into a src directory that would be perfect.

Okay! Files moved in 152858a.

kairos-cli/src/crypto/private_key.rs

kairos-cli/src/crypto/signer.rs

Avi-D-coder · 2024-02-16T19:01:45Z

LGTM

marijanp

LGTM

quinn-dougherty

LGTM

kairos-cli/src/crypto/signer.rs

xcthulhu · 2024-02-20T15:46:57Z

kairos-cli/src/commands/deposit.rs

@@ -0,0 +1,26 @@
+use crate::common::args::{AmountArg, PrivateKeyPathArg};
+use crate::crypto::error::CryptoError;
+use crate::crypto::signer::CasperSigner;


Why did we make a custom signer?

I want to isolate Casper code, to make it easily replaceable in the future. I imagine we might have Signer trait and blockchain specific implementations for it.

xcthulhu · 2024-02-20T15:52:10Z

kairos-cli/src/crypto/public_key.rs

+pub struct CasperPublicKey(pub casper_types::PublicKey);
+
+impl CasperPublicKey {
+    pub fn from_bytes(bytes: &[u8]) -> Result<Self, CryptoError> {


Don’t make this part of the inherent implementation, just impl the FromBytes trait if you want to do this

FromBytes is a trait existing in casper_types, and my intention was to make Kairos as much independent as possible.

It can be constructed directly via public field.

Better separation with executable at `./bin/main.rs`.

More precise errors for caller, with the cost of manual casting into `CryptoError` in handlers.

koxu1996 · 2024-02-26T12:46:36Z

Changes rebased with the latest main.

Public keys were taken directly from: - https://cspr.live/account/01c377281132044bd3278b039925eeb3efdb9d99dd5f46d9ec6a764add34581af7 - https://cspr.live/account/0202e99759649fa63a72c685b72e696b30c90f1deabb02d0d9b1de45eb371a73e5bb

koxu1996 · 2024-02-29T10:51:58Z

Per @xcthulhu request, I added additional tests for parsing public keys from CSPR.live. All four approvals were automatically dismissed though 😨.

@asladeofgreen PTAL 🙏

kairos-cli/src/crypto/public_key.rs

Now it is possible to differentiate serialization and deserialization error.

Avi-D-coder

lgtm

asladeofgreen

I have caveats, notably with reinventing wheels in respect of the cryptography, however I do understand the wish to reduce dependency upon casper libs as much as possible. Wouldn't a dedicated low-level kairos-crypto crate be of use here ?

So am approving for now but imho would like to revisit some of this code as the solution evolves.

koxu1996 · 2024-03-15T10:06:14Z

I have caveats, notably with reinventing wheels in respect of the cryptography, however I do understand the wish to reduce dependency upon casper libs as much as possible. Wouldn't a dedicated low-level kairos-crypto crate be of use here ?

@asladeofgreen Yes, cryptography will be moved out of CLI package into kairos-crypto in #35. It provides pub trait CryptoSigner, with Casper implementation under feature flag.

koxu1996 marked this pull request as ready for review February 6, 2024 13:53

koxu1996 requested review from marijanp, Avi-D-coder and asladeofgreen February 6, 2024 13:54

Avi-D-coder requested changes Feb 6, 2024

View reviewed changes

kairos-cli/bin/commands/mod.rs Outdated Show resolved Hide resolved

kairos-cli/bin/main.rs Outdated Show resolved Hide resolved

kairos-cli/tests/cli_tests.rs Outdated Show resolved Hide resolved

koxu1996 requested a review from Avi-D-coder February 6, 2024 21:03

Avi-D-coder approved these changes Feb 6, 2024

View reviewed changes

koxu1996 force-pushed the feature/base-cli-parsing branch 3 times, most recently from 89955b1 to ade64d1 Compare February 6, 2024 22:40

marijanp reviewed Feb 7, 2024

View reviewed changes

koxu1996 requested review from marijanp and Avi-D-coder February 12, 2024 17:48

Avi-D-coder reviewed Feb 13, 2024

View reviewed changes

kairos-cli/bin/commands/transfer.rs Outdated Show resolved Hide resolved

Avi-D-coder requested changes Feb 13, 2024

View reviewed changes

marijanp reviewed Feb 13, 2024

View reviewed changes

koxu1996 requested review from Avi-D-coder and marijanp February 13, 2024 19:02

Avi-D-coder requested changes Feb 13, 2024

View reviewed changes

kairos-cli/src/crypto/private_key.rs Outdated Show resolved Hide resolved

kairos-cli/src/crypto/signer.rs Outdated Show resolved Hide resolved

koxu1996 self-assigned this Feb 15, 2024

koxu1996 requested a review from Avi-D-coder February 16, 2024 13:01

Avi-D-coder previously approved these changes Feb 16, 2024

View reviewed changes

marijanp previously approved these changes Feb 17, 2024

View reviewed changes

quinn-dougherty previously approved these changes Feb 17, 2024

View reviewed changes

kairos-cli/src/crypto/signer.rs Show resolved Hide resolved

xcthulhu reviewed Feb 20, 2024

View reviewed changes

koxu1996 added 5 commits February 26, 2024 13:44

Expose signer's field public_key directly.

a9ed198

Remove CasperPublicKey::from_key().

8eb6e69

It can be constructed directly via public field.

Remove outdated TODO.

55879c2

Moved core logic into src directory.

560645a

Better separation with executable at `./bin/main.rs`.

Return unwrapped ErrorExt in crypto constructors.

455872d

More precise errors for caller, with the cost of manual casting into `CryptoError` in handlers.

koxu1996 force-pushed the feature/base-cli-parsing branch from 5556046 to 455872d Compare February 26, 2024 12:45

koxu1996 requested a review from xcthulhu February 26, 2024 12:46

xcthulhu previously approved these changes Feb 26, 2024

View reviewed changes

Add tests for parsing Casper public keys.

3775a71

Public keys were taken directly from: - https://cspr.live/account/01c377281132044bd3278b039925eeb3efdb9d99dd5f46d9ec6a764add34581af7 - https://cspr.live/account/0202e99759649fa63a72c685b72e696b30c90f1deabb02d0d9b1de45eb371a73e5bb

koxu1996 dismissed stale reviews from xcthulhu, quinn-dougherty, Avi-D-coder, and marijanp via 3775a71 February 29, 2024 10:47

koxu1996 requested review from xcthulhu and quinn-dougherty February 29, 2024 10:52

Fix formatting with cargo fmt.

1eb91d4

marijanp previously approved these changes Mar 5, 2024

View reviewed changes

kairos-cli/src/crypto/public_key.rs Outdated Show resolved Hide resolved

kairos-cli/src/crypto/public_key.rs Outdated Show resolved Hide resolved

Make serialization error context more specific.

2a37718

Now it is possible to differentiate serialization and deserialization error.

koxu1996 dismissed marijanp’s stale review via 2a37718 March 5, 2024 14:19

koxu1996 requested review from marijanp and Avi-D-coder March 5, 2024 14:20

marijanp approved these changes Mar 6, 2024

View reviewed changes

Avi-D-coder approved these changes Mar 11, 2024

View reviewed changes

asladeofgreen approved these changes Mar 14, 2024

View reviewed changes

koxu1996 merged commit 948f15a into main Mar 15, 2024
6 checks passed

koxu1996 deleted the feature/base-cli-parsing branch March 18, 2024 09:38

koxu1996 mentioned this pull request Mar 18, 2024

Signer module - kairos-crypto #35

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Base CLI parsing #17

Base CLI parsing #17

koxu1996 commented Feb 5, 2024 •

edited

Avi-D-coder left a comment

koxu1996 commented Feb 6, 2024 •

edited

marijanp left a comment

koxu1996 commented Feb 12, 2024 •

edited

marijanp left a comment

koxu1996 commented Feb 13, 2024

Avi-D-coder commented Feb 16, 2024

marijanp left a comment

quinn-dougherty left a comment

xcthulhu Feb 20, 2024

koxu1996 Feb 21, 2024

xcthulhu Feb 20, 2024

koxu1996 Feb 21, 2024

koxu1996 commented Feb 26, 2024

koxu1996 commented Feb 29, 2024

Avi-D-coder left a comment

asladeofgreen left a comment

koxu1996 commented Mar 15, 2024

Base CLI parsing #17

Base CLI parsing #17

Conversation

koxu1996 commented Feb 5, 2024 • edited

CLI interface

Outputs

Error handling

Tests

Avi-D-coder left a comment

Choose a reason for hiding this comment

koxu1996 commented Feb 6, 2024 • edited

marijanp left a comment

Choose a reason for hiding this comment

koxu1996 commented Feb 12, 2024 • edited

marijanp left a comment

Choose a reason for hiding this comment

koxu1996 commented Feb 13, 2024

Avi-D-coder commented Feb 16, 2024

marijanp left a comment

Choose a reason for hiding this comment

quinn-dougherty left a comment

Choose a reason for hiding this comment

xcthulhu Feb 20, 2024

Choose a reason for hiding this comment

koxu1996 Feb 21, 2024

Choose a reason for hiding this comment

xcthulhu Feb 20, 2024

Choose a reason for hiding this comment

koxu1996 Feb 21, 2024

Choose a reason for hiding this comment

koxu1996 commented Feb 26, 2024

koxu1996 commented Feb 29, 2024

Avi-D-coder left a comment

Choose a reason for hiding this comment

asladeofgreen left a comment

Choose a reason for hiding this comment

koxu1996 commented Mar 15, 2024

koxu1996 commented Feb 5, 2024 •

edited

koxu1996 commented Feb 6, 2024 •

edited

koxu1996 commented Feb 12, 2024 •

edited