Jcli make transaction #3230

danielSanchezQ · 2021-04-20T09:37:59Z

I'm starting this as a draft. It is a close example of what jcli path to being a lib should lead to.

The simplified transaction command uses several of the other transaction commands.
I started extracting the common code into external functions. Also stripping some functionality like reading and writing to files that shouldn't be within the reusable computations.

danielSanchezQ · 2021-04-20T09:38:32Z

This is related to #3216

jcli/src/jcli_lib/transaction/simplified.rs

jcli/src/jcli_lib/transaction/mod.rs

jcli/src/jcli_lib/address.rs

jcli/src/jcli_lib/transaction/mod.rs

jcli/src/jcli_lib/address.rs

jcli/src/jcli_lib/rest/config.rs

jcli/src/jcli_lib/rest/v0/account/mod.rs

eugene-babichenko · 2021-04-23T08:53:04Z

jcli/src/jcli_lib/transaction/add_account.rs

+pub fn add_account(
+    account: interfaces::Address,
+    value: interfaces::Value,
+    transaction: &mut Staging,


Is Staging defined in jcli? If so, this can be done into Staging::account and this would be much cleaner.

EDIT: now I see that this is common approach in this crate and we should make a note for future us to refactor that.

jcli/src/jcli_lib/transaction/mod.rs

eugene-babichenko · 2021-04-23T08:57:58Z

jcli/src/jcli_lib/transaction/simplified.rs

+    Ok((sk, address))
+}
+
+#[allow(clippy::too_many_arguments)]


Clippy has got a point here :)

It has indeed but is what is needed. Even if splitting it would have to receive such data. I'll try to do something to reduce that anyway.

I could refactor things inside in some smaller sections maybe. But the grouping function would still need to receive that. At the end we need all we have in the cli input to compute :/

jcli/src/jcli_lib/transaction/simplified.rs

eugene-babichenko

grep-ing (or rg-ing for fellow Rustaceans) still shows notions of faucet and "simplified transactions".

eugene-babichenko · 2021-04-26T09:00:05Z

Definitely not how errors should be reported.

thread 'main' panicked at 'Found positional argument which is not required with a lower index than a required positional argument: "change" index 4', /home/babichenko/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.3/src/app/parser.rs:643:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

UPD: wait, this happening even for jcli transaction make-transaction --help

eugene-babichenko · 2021-04-26T09:11:25Z

The help itself is fine and accessible via jcli help transaction make-transaction. This must be an issue with the library.

danielSanchezQ · 2021-04-26T10:53:03Z

The failing test looks not related.

eugene-babichenko · 2021-04-27T07:19:08Z

It works 👍. Here are my concerns:

It outputs Private key of receiver (to revert transaction for testing purposes) and I would rather remove it for security reasons.
We do actual transaction sending here, but (see the word "offline")

	❯ jcli transaction --help
	jcli-transaction 0.11.1
	Build and view offline transaction

I think it might be better to set the default values of fees to the Jormungandr defaults.
The help is still sort of broken (some arguments not showing) and I think this is due to structure fields ordering again.

	❯ jcli transaction make-transaction -h
	error: The following required arguments were not provided:
    	<ACCOUNT>
    	<VALUE>
    	<block0-hash>
	
	USAGE:

danielSanchezQ · 2021-04-27T07:34:47Z

@eugene-babichenko how about?:

It outputs Private key of receiver (to revert transaction for testing purposes) and I would rather remove it for security reasons.

Maybe add flag to enable disable it?

We do actual transaction sending here, but (see the word "offline")

Didn't understand this. Could you elaborate?

I think it might be better to set the default values of fees to the Jormungandr defaults.

Definetly. Can you point me to the values please?

The help is still sort of broken (some arguments not showing) and I think this is due to structure fields ordering again.

wtf...it actually works with --help 😞

eugene-babichenko · 2021-04-27T07:57:24Z

Maybe add flag to enable disable it?

Just remove IMO.

Didn't understand this. Could you elaborate?

Everything under jcli transactions is dedicated to building transactions and do not contact HTTP endpoints.

Definetly. Can you point me to the values please?

Uhm, well, I guess we don't have defaults for them, or I still did not wake up. Will need to ask.

eugene-babichenko · 2021-04-29T07:30:41Z

jcli/src/jcli_lib/transaction/simplified.rs

+    pub fee: common::CommonFees,
+
+    #[structopt(flatten)]
+    rest_args: RestArgs,


They need to be removed since we don't do transaction sending here anymore.

It is still needed to query some account data.

Oopsie, sorry.

danielSanchezQ · 2021-04-30T15:37:30Z

Added request fees from rest api

eugene-babichenko · 2021-05-28T06:57:24Z

testing/jormungandr-integration-tests/src/jcli/transaction/simplified.rs

+        block0_hash.to_string(),
+        sk_file_path,
+        staging_file,
+    );


Shall we also make sure the transaction was successfully casted?

It is internally asserted in the make_transaction method.

Added make witness

Missing account....

Refactored keys+address methods

…ory arguments

Removed send transaction Store transaction instead

Adapt tests

Add test for fees handling

Added test for post fragment

danielSanchezQ added enhancement New feature or request technical debt issue to track potential technical dept we are introducing but will need care later. A-jcli Area: Command Line Interface and tooling labels Apr 20, 2021

danielSanchezQ requested review from eugene-babichenko, filip-dulic-bloxico and a team April 20, 2021 09:37

danielSanchezQ self-assigned this Apr 20, 2021

ecioppettini reviewed Apr 20, 2021

View reviewed changes

jcli/src/jcli_lib/transaction/simplified.rs Outdated Show resolved Hide resolved

ecioppettini reviewed Apr 20, 2021

View reviewed changes

jcli/src/jcli_lib/transaction/mod.rs Outdated Show resolved Hide resolved

danielSanchezQ marked this pull request as ready for review April 23, 2021 08:04

danielSanchezQ commented Apr 23, 2021

View reviewed changes

jcli/src/jcli_lib/address.rs Outdated Show resolved Hide resolved

danielSanchezQ commented Apr 23, 2021

View reviewed changes

jcli/src/jcli_lib/transaction/mod.rs Outdated Show resolved Hide resolved

eugene-babichenko reviewed Apr 23, 2021

View reviewed changes

danielSanchezQ requested review from eugene-babichenko and ecioppettini April 23, 2021 13:14

eugene-babichenko suggested changes Apr 24, 2021

View reviewed changes

danielSanchezQ requested a review from eugene-babichenko April 26, 2021 14:01

eugene-babichenko reviewed Apr 29, 2021

View reviewed changes

danielSanchezQ changed the title ~~Jcli simplified transaction~~ Jcli make transaction Apr 30, 2021

danielSanchezQ requested a review from eugene-babichenko May 12, 2021 11:15

danielSanchezQ force-pushed the jcli-simplified-transaction branch from 541c731 to 3e6b31e Compare May 26, 2021 07:55

eugene-babichenko reviewed May 28, 2021

View reviewed changes

danielsanchezq added 26 commits July 5, 2021 10:51

Added retrieve account information

f13b660

Added make witness

Added send fragment

deb6aa9

Get keys ready

ab4633f

Missing account....

Rename command to simplified

33d7268

Cleanup imports

37724ae

Refactored keys+address methods

Rename and clean

964b167

Missed faucet cleaning

67b304b

Missed simplified cleaning

97fcbfc

Refactored Staging add_account into a struct method

caab642

Reorder arguments to avoid structopt order issues when parsing mandat…

0773aba

…ory arguments

Removed output

58c30c9

Removed send transaction Store transaction instead

Request fees through rest API

03f1ef9

Added make transaction integration test

20b1df9

Add receiver account as optional parameter

1b6f558

Adapt tests

Allow clippy many arguments for make_transaction methods

d23c218

Refactor: receiver-account -> receiver

d245aca

Add proper fees handling

30bb77c

Add test for fees handling

Fit fees and values closer in test

ffb5b77

Trim input endline

3905529

Implement post fragment from make-transaction

4e455b7

Added test for post fragment

Fix prints issues

bf15dbd

Fix printl typo

b0b4f1a

Fix wrong printing when asking input

7aa8e45

Use interactive secret key read

0eb6caf

Refactor not save to file when transaction is directly posted

3af372e

Update cargo lock with missing deps

227e1a4

danielSanchezQ force-pushed the jcli-simplified-transaction branch from 4c6909a to 227e1a4 Compare July 5, 2021 08:57

Fix clippy

4d82704

eugene-babichenko approved these changes Jul 8, 2021

View reviewed changes

eugene-babichenko merged commit e373202 into input-output-hk:master Jul 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jcli make transaction #3230

Jcli make transaction #3230

danielSanchezQ commented Apr 20, 2021

danielSanchezQ commented Apr 20, 2021

eugene-babichenko Apr 23, 2021

eugene-babichenko Apr 23, 2021

danielSanchezQ Apr 23, 2021

danielSanchezQ Apr 23, 2021

eugene-babichenko left a comment

eugene-babichenko commented Apr 26, 2021 •

edited

Loading

eugene-babichenko commented Apr 26, 2021

danielSanchezQ commented Apr 26, 2021 •

edited

Loading

eugene-babichenko commented Apr 27, 2021

danielSanchezQ commented Apr 27, 2021

eugene-babichenko commented Apr 27, 2021

eugene-babichenko Apr 29, 2021

danielSanchezQ Apr 29, 2021

eugene-babichenko Apr 29, 2021

danielSanchezQ commented Apr 30, 2021

eugene-babichenko May 28, 2021

danielSanchezQ May 28, 2021

Jcli make transaction #3230

Jcli make transaction #3230

Conversation

danielSanchezQ commented Apr 20, 2021

danielSanchezQ commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugene-babichenko left a comment

Choose a reason for hiding this comment

eugene-babichenko commented Apr 26, 2021 • edited Loading

eugene-babichenko commented Apr 26, 2021

danielSanchezQ commented Apr 26, 2021 • edited Loading

eugene-babichenko commented Apr 27, 2021

danielSanchezQ commented Apr 27, 2021

eugene-babichenko commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielSanchezQ commented Apr 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugene-babichenko commented Apr 26, 2021 •

edited

Loading

danielSanchezQ commented Apr 26, 2021 •

edited

Loading