Refactor CLI by adding add new click types for fees, amounts, addresses and bytes32 #15718

jack60612 · 2023-07-09T04:30:13Z

Purpose:

Adds new TransactionFeeParamType for automatic fee conversion
Adds new CliAmount class and AmountParamType for easy conversion between units.
Adds new CliAddress class and AddressParamType for easy bech32m address conversion and wallet type tracking
Adds new Bytes32ParamType for automatic str to bytes32 conversion.
Lastly this pr does more standardization too.

Merge requirements:

Cleanup CLI RPC connection error handling #15704 Needs to be Merged
PR needs to be rebased on main after merge
~~Some light testing of the --mojos argument~~ (Moved to future PR)
Add Unit Tests
Cli tests added by Add Wallet CLI Unit tests #15886 Merged

Current Behavior:

No Breaking Changes, just lots of backend changes.

New Behavior:

N/A

Testing Notes:

This PR has unit tests for the new types, and actual cli tests are coming in #15718

github-actions · 2023-07-10T20:42:32Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2023-07-10T23:27:50Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

arvidn

I appreciate this integration with Click. I think the new classes should have unit tests though.

chia/cmds/param_types.py

wallentx · 2023-07-17T17:40:08Z

Failure. Coverage is below 100%.

Diff Coverage
Diff: origin/main...HEAD, staged and unstaged changes

chia/cmds/coin_funcs.py (0.0%): Missing lines 7,44-45,109,111,143-144,148,184,188-189,220,223
chia/cmds/coins.py (100%)
chia/cmds/data.py (100%)
chia/cmds/data_funcs.py (23.5%): Missing lines 17,24,35,45,55,64,74,83,93,104,113,153,174
chia/cmds/param_types.py (92.9%): Missing lines 30-33,78-81
chia/cmds/plotnft.py (85.7%): Missing lines 84
chia/cmds/plotnft_funcs.py (0.0%): Missing lines 19,29,36,327,344,355,358
chia/cmds/wallet.py (84.8%): Missing lines 585,612,665,1303-1304
chia/cmds/wallet_funcs.py (16.7%): Missing lines 374-379,381-383,652,970,1015,1043-1044,1129,1286,1288,1337,1344,1349
chia/rpc/wallet_rpc_client.py (100%)
tests/cmds/test_types.py (100%)
tests/pools/test_pool_cmdline.py (100%)

Total: 318 lines
Missing: 67 lines
Coverage: 78%

jack60612 · 2023-07-17T18:10:58Z

Failure. Coverage is below 100%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

chia/cmds/coin_funcs.py (0.0%): Missing lines 7,44-45,109,111,143-144,148,184,188-189,220,223

chia/cmds/coins.py (100%)
chia/cmds/data.py (100%)
chia/cmds/data_funcs.py (23.5%): Missing lines 17,24,35,45,55,64,74,83,93,104,113,153,174
chia/cmds/param_types.py (92.9%): Missing lines 30-33,78-81
chia/cmds/plotnft.py (85.7%): Missing lines 84
chia/cmds/plotnft_funcs.py (0.0%): Missing lines 19,29,36,327,344,355,358
chia/cmds/wallet.py (84.8%): Missing lines 585,612,665,1303-1304
chia/cmds/wallet_funcs.py (16.7%): Missing lines 374-379,381-383,652,970,1015,1043-1044,1129,1286,1288,1337,1344,1349
chia/rpc/wallet_rpc_client.py (100%)
tests/cmds/test_types.py (100%)
tests/pools/test_pool_cmdline.py (100%)

Total: 318 lines

Missing: 67 lines
Coverage: 78%

this only tests the new types class, and the missing lines in in param funcs are expected, as the mojos part isn't user available yet.

arvidn

looks good. Could you expand a bit on why you need the isinstance in the convert functions? it looks risky. If it's just for the default value, would it work yo change the type to the default value to str to look just like what would be passed on the command line?

chia/cmds/coin_funcs.py

chia/cmds/param_types.py

arvidn · 2023-07-26T13:57:05Z

It would make sense to hold off landing this until we have test coverage, right?

Diff: origin/main...HEAD, staged and unstaged changes
-------------
chia/cmds/coin_funcs.py (0.0%): Missing lines 7,44-45,109,111,143-144,148,184,188-189,220,223
chia/cmds/coins.py (100%)
chia/cmds/data.py (100%)
chia/cmds/data_funcs.py (23.5%): Missing lines 17,24,35,45,55,64,74,83,93,104,113,153,174
chia/cmds/param_types.py (92.9%): Missing lines 30-33,78-81
chia/cmds/plotnft.py (85.7%): Missing lines 84
chia/cmds/plotnft_funcs.py (0.0%): Missing lines 19,29,36,327,344,355,358
chia/cmds/wallet.py (84.8%): Missing lines 585,612,665,1303-1304
chia/cmds/wallet_funcs.py (16.7%): Missing lines 374-379,381-383,652,970,1015,1043-1044,1129,1286,1288,1337,1344,1349
chia/rpc/wallet_rpc_client.py (100%)
tests/cmds/test_types.py (100%)
tests/pools/test_pool_cmdline.py (100%)

jack60612 · 2023-07-26T15:30:33Z

It would make sense to hold off landing this until we have test coverage, right?

Diff: origin/main...HEAD, staged and unstaged changes
-------------
chia/cmds/coin_funcs.py (0.0%): Missing lines 7,44-45,109,111,143-144,148,184,188-189,220,223
chia/cmds/coins.py (100%)
chia/cmds/data.py (100%)
chia/cmds/data_funcs.py (23.5%): Missing lines 17,24,35,45,55,64,74,83,93,104,113,153,174
chia/cmds/param_types.py (92.9%): Missing lines 30-33,78-81
chia/cmds/plotnft.py (85.7%): Missing lines 84
chia/cmds/plotnft_funcs.py (0.0%): Missing lines 19,29,36,327,344,355,358
chia/cmds/wallet.py (84.8%): Missing lines 585,612,665,1303-1304
chia/cmds/wallet_funcs.py (16.7%): Missing lines 374-379,381-383,652,970,1015,1043-1044,1129,1286,1288,1337,1344,1349
chia/rpc/wallet_rpc_client.py (100%)
tests/cmds/test_types.py (100%)
tests/pools/test_pool_cmdline.py (100%)

In my mind this increases overall coverage by centralizing where everything is converted, but we can hold it off if you want those other tests first.

github-actions · 2023-07-27T17:32:08Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

also remove validate fee callback from wallet

tests/cmds/test_click_types.py

chia/cmds/param_types.py

altendky

I didn't repeat a detailed review yet. Let's address the existing open issues then go back over the whole thing.

Co-authored-by: Kyle Altendorf <sda@fstab.net>

github-actions · 2023-11-25T18:43:35Z

File	Coverage	Missing Lines
`chia/cmds/coin_funcs.py`	75.0%	lines 106, 108
`chia/cmds/data_funcs.py`	0.0%	lines 45, 58, 70, 81, 92, 102, 113, 124, 135, 147, 157, 215, 243
`chia/cmds/param_types.py`	93.3%	lines 33-34, 74, 80-81, 126-128, 165, 202
`chia/cmds/plotnft.py`	90.9%	lines 79
`chia/cmds/plotnft_funcs.py`	0.0%	lines 19, 29, 36, 340, 357, 368, 371
`chia/cmds/wallet_funcs.py`	82.1%	lines 295, 299, 301-302, 662, 1398, 1552

Total	Missing	Coverage
484 lines	40 lines	91%

arvidn

looks reasonable. except it looks like that block of code in validate_uint64() could be removed

chia/cmds/param_types.py

altendky

Looks like a couple still open conversations to finish with.

chia/cmds/param_types.py

just meant to comment, not approve. sorry

altendky

previous approval was meant to be a comment

arvidn · 2024-01-06T10:18:53Z

the cmd test on windows failed twice in a row (with what looks like a time out). I would expect all of these tests to be 100% deterministic, and not be flaky.

arvidn · 2024-01-06T11:21:30Z

https://github.com/Chia-Network/chia-blockchain/actions/runs/7414705354/job/20221244941?pr=15718

altendky

alrighty, this covers the param types and the test file for them. i'll try to get to the rest of the files, but expect them to be a lot less interesting.

altendky · 2024-01-06T01:43:14Z

chia/cmds/param_types.py

+    try:
+        d_value = Decimal(value)
+    except InvalidOperation:
+        fail_func("Value must be a valid uint64 number", param, ctx)


uint64 isn't the proper description here. Should also have the actual failure probably, as noted elsewhere.

altendky · 2024-01-06T01:52:16Z

chia/cmds/param_types.py

+) -> Decimal:
+    try:
+        d_value = Decimal(value)
+    except InvalidOperation:  # won't ever be value error because of the fee limit check


This comment doesn't seem applicable.

Suggested change

except InvalidOperation: # won't ever be value error because of the fee limit check

except InvalidOperation:

altendky · 2024-01-06T02:13:52Z

chia/cmds/param_types.py

+        fail_func("Value must be decimal dotted value in XCH (e.g. 0.00005)", param, ctx)
+    if d_value.is_signed():
+        fail_func("Value can not be negative", param, ctx)
+    if not d_value.is_zero() and d_value < Decimal("0.000000000001"):


This still allows, for example, 0.0000000000015 which isn't valid. Unless there is a specific feature for this, we might have to multiply by whatever-our-xch-mojo-conversion-constant is then .to_integral() and compare.

Alternatively, we delete this conversion function and just multiply by the xch-mojo conversion when needed and then use the validate-uint64. The caveat being that ... iirc, balances can be uint128 but coin amounts can only be uint64. So maybe there's a relevant detail there.

altendky · 2024-01-06T02:22:14Z

chia/cmds/param_types.py

+    A Click parameter type for transaction fees, which can be specified in XCH or mojos.
+    """
+
+    name: str = uint64.__name__  # output type


Technically these are all ClassVars such as name: ClassVar[str]. Probably the same across all these classes.

altendky · 2024-01-06T15:41:08Z

chia/cmds/param_types.py

+    A Click parameter type for transaction fees, which can be specified in XCH or mojos.
+    """
+
+    name: str = uint64.__name__  # output type


Aside from the hinting, I'm not sure this is right. I did suggest uint64.__name__ instead of "uint64" since it seemed to be the intent to be tied to the Python type we convert to. But, I'm not entirely sure that we want that as the name.

https://click.palletsprojects.com/en/8.1.x/api/#click.ParamType.name

the descriptive name of this type

Maybe we should figure out where this is used so we can make sure we have useful text there. Or maybe uint64 still ends up sufficiently useful depending.

Or, we can just keep this in mind for later consideration.

altendky · 2024-01-06T21:27:31Z

chia/cmds/param_types.py

+                    from chia.util.config import load_config
+                    from chia.util.default_root import DEFAULT_ROOT_PATH


Is there a need for these to not be global scope imports?

altendky · 2024-01-06T22:11:08Z

chia/cmds/param_types.py

+                if hrp == expected_prefix:
+                    addr_type = AddressType.XCH
+                else:
+                    self.fail(f"Unexpected Address Prefix: {hrp}, are you sure its for the right network?", param, ctx)
+            else:
+                addr_type = AddressType(hrp)


Feel free to close this after reading.

It feels like this pattern belongs on the AddressType class. The 'to hrp' helper already takes a config so depending on that wouldn't be an issue. It should probably also provide the xch/txch options in some way. An idea for another time.

altendky · 2024-01-06T22:30:41Z

tests/cmds/test_click_types.py

+    # TODO: Test MOJO Logic When Implemented
+
+    # Test Decimal / XCH
+    assert TransactionFeeParamType().convert("0.5", None, None) == uint64(int(0.5 * units["chia"]))


Suggested change

assert TransactionFeeParamType().convert("0.5", None, None) == uint64(int(0.5 * units["chia"]))

assert TransactionFeeParamType().convert("0.5", None, None) == uint64(Decimal("0.5") * units["chia"])

altendky · 2024-01-06T22:33:17Z

tests/cmds/test_click_types.py

+        AmountParamType().convert("-0.6", None, None)
+    # Test Type Failures
+    with pytest.raises(BadParameter):
+        AmountParamType().convert(float(0.01), None, None)


Suggested change

AmountParamType().convert(float(0.01), None, None)

AmountParamType().convert(0.01, None, None)

altendky · 2024-01-06T22:33:41Z

tests/cmds/test_click_types.py

+        AmountParamType().convert(float(0.01), None, None)
+
+    # Test CliAmount Class
+    assert decimal_cli_amount.convert_amount(units["chia"]) == uint64(int(5.25 * units["chia"]))


Suggested change

assert decimal_cli_amount.convert_amount(units["chia"]) == uint64(int(5.25 * units["chia"]))

assert decimal_cli_amount.convert_amount(units["chia"]) == uint64(Decimal("5.25") * units["chia"])

github-actions · 2024-01-16T15:24:50Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2024-01-16T15:24:50Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

wjblanke · 2024-03-13T16:40:42Z

Lets discuss this again with #16802

jack60612 added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog CI CI changes labels Jul 9, 2023

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 10, 2023

jack60612 changed the title ~~Add --mojos argument to the CLI to allow the use of mojo instead of xch, and add lots of new classes as part of the CLI refactor~~ Refactor CLI Classes and add new click types for fees, amounts, addresses and bytes32 Jul 10, 2023

jack60612 changed the title ~~Refactor CLI Classes and add new click types for fees, amounts, addresses and bytes32~~ Refactor CLI by adding add new click types for fees, amounts, addresses and bytes32 with auto conversion in each class. Jul 10, 2023

jack60612 force-pushed the jn.cli-rework-types branch from 52186b1 to 4c7317e Compare July 10, 2023 23:27

github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jul 10, 2023

jack60612 changed the title ~~Refactor CLI by adding add new click types for fees, amounts, addresses and bytes32 with auto conversion in each class.~~ Refactor CLI by adding add new click types for fees, amounts, addresses and bytes32 Jul 10, 2023

jack60612 marked this pull request as ready for review July 11, 2023 02:12

jack60612 requested a review from a team as a code owner July 11, 2023 02:12

jack60612 requested a review from arvidn July 11, 2023 02:12

arvidn reviewed Jul 11, 2023

View reviewed changes

chia/cmds/param_types.py Outdated Show resolved Hide resolved

chia/cmds/param_types.py Show resolved Hide resolved

jack60612 force-pushed the jn.cli-rework-types branch from 4c7317e to 1ff5910 Compare July 11, 2023 21:32

jack60612 requested a review from arvidn July 11, 2023 21:33

jack60612 force-pushed the jn.cli-rework-types branch from 1ff5910 to cf6c0fc Compare July 14, 2023 16:45

arvidn reviewed Jul 25, 2023

View reviewed changes

chia/cmds/coin_funcs.py Show resolved Hide resolved

chia/cmds/param_types.py Outdated Show resolved Hide resolved

chia/cmds/param_types.py Outdated Show resolved Hide resolved

chia/cmds/param_types.py Outdated Show resolved Hide resolved

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 27, 2023

jack60612 mentioned this pull request Jul 28, 2023

Add Wallet CLI Unit tests #15886

Merged

jack60612 added 5 commits August 14, 2023 13:06

Add new click Fee, Amount, Bytes32 and Address Types with unit support.

ffa14e8

Add new amount and fee types to coin commands.

8db5864

Add new fee and bytes32 types to datalayer commands.

a9e1409

update plotnft cmds with new fee and address types.

20f7bbc

also remove validate fee callback from wallet

update wallet commands with new fee, amount, address and bytes32 types.

8eb37bd

github-actions bot added the coverage-diff label Nov 21, 2023

altendky reviewed Nov 25, 2023

View reviewed changes

tests/cmds/test_click_types.py Outdated Show resolved Hide resolved

tests/cmds/test_click_types.py Outdated Show resolved Hide resolved

chia/cmds/param_types.py Outdated Show resolved Hide resolved

chia/cmds/param_types.py Outdated Show resolved Hide resolved

altendky requested changes Nov 25, 2023

View reviewed changes

jack60612 self-assigned this Nov 25, 2023

jack60612 dismissed arvidn’s stale review via 79026c3 November 25, 2023 16:11

Apply suggestions from code review

79026c3

Co-authored-by: Kyle Altendorf <sda@fstab.net>

jack60612 mentioned this pull request Nov 25, 2023

Fixed handling for max coin amount when it is set to zero & updated unit tests #16893

Closed

jack60612 linked an issue Nov 25, 2023 that may be closed by this pull request

[Bug] Combining coins with fee #16802

Open

address changes, add new tests and reduce code duplication

483b279

jack60612 requested a review from altendky November 25, 2023 17:54

fix type for default in dao

1a20311

github-actions bot removed the coverage-diff label Nov 25, 2023

github-actions bot added the coverage-diff label Nov 25, 2023

expand tesssts

f088211

arvidn reviewed Jan 3, 2024

View reviewed changes

chia/cmds/param_types.py Show resolved Hide resolved

altendky previously approved these changes Jan 4, 2024

View reviewed changes

chia/cmds/param_types.py Outdated Show resolved Hide resolved

chia/cmds/param_types.py Outdated Show resolved Hide resolved

altendky requested changes Jan 4, 2024

View reviewed changes

address changes

f1ffda4

jack60612 requested review from arvidn and altendky January 4, 2024 21:06

altendky requested changes Jan 6, 2024

View reviewed changes

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jan 16, 2024

altendky mentioned this pull request Feb 7, 2024

[CHIA-435] Add chia wallet signer commands #17399

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CLI by adding add new click types for fees, amounts, addresses and bytes32 #15718

Refactor CLI by adding add new click types for fees, amounts, addresses and bytes32 #15718

jack60612 commented Jul 9, 2023 •

edited

github-actions bot commented Jul 10, 2023

github-actions bot commented Jul 10, 2023

arvidn left a comment

wallentx commented Jul 17, 2023

jack60612 commented Jul 17, 2023

Failure. Coverage is below 100%.

Diff Coverage

chia/cmds/coin_funcs.py (0.0%): Missing lines 7,44-45,109,111,143-144,148,184,188-189,220,223

Total: 318 lines

arvidn left a comment

arvidn commented Jul 26, 2023 •

edited

jack60612 commented Jul 26, 2023

github-actions bot commented Jul 27, 2023

altendky left a comment

github-actions bot commented Nov 25, 2023

arvidn left a comment

altendky left a comment

altendky left a comment

arvidn commented Jan 6, 2024

arvidn commented Jan 6, 2024

altendky left a comment

altendky Jan 6, 2024

altendky Jan 6, 2024

altendky Jan 6, 2024

altendky Jan 6, 2024

altendky Jan 6, 2024

altendky Jan 6, 2024

altendky Jan 6, 2024

altendky Jan 6, 2024

altendky Jan 6, 2024

altendky Jan 6, 2024

github-actions bot commented Jan 16, 2024

github-actions bot commented Jan 16, 2024

wjblanke commented Mar 13, 2024

	except InvalidOperation: # won't ever be value error because of the fee limit check
	except InvalidOperation:

		from chia.util.config import load_config
		from chia.util.default_root import DEFAULT_ROOT_PATH

	assert TransactionFeeParamType().convert("0.5", None, None) == uint64(int(0.5 * units["chia"]))
	assert TransactionFeeParamType().convert("0.5", None, None) == uint64(Decimal("0.5") * units["chia"])

	AmountParamType().convert(float(0.01), None, None)
	AmountParamType().convert(0.01, None, None)

	assert decimal_cli_amount.convert_amount(units["chia"]) == uint64(int(5.25 * units["chia"]))
	assert decimal_cli_amount.convert_amount(units["chia"]) == uint64(Decimal("5.25") * units["chia"])

Refactor CLI by adding add new click types for fees, amounts, addresses and bytes32 #15718

Are you sure you want to change the base?

Refactor CLI by adding add new click types for fees, amounts, addresses and bytes32 #15718

Conversation

jack60612 commented Jul 9, 2023 • edited

Purpose:

Current Behavior:

New Behavior:

Testing Notes:

github-actions bot commented Jul 10, 2023

github-actions bot commented Jul 10, 2023

arvidn left a comment

Choose a reason for hiding this comment

wallentx commented Jul 17, 2023

Failure. Coverage is below 100%.

Diff Coverage Diff: origin/main...HEAD, staged and unstaged changes

Total: 318 lines Missing: 67 lines Coverage: 78%

jack60612 commented Jul 17, 2023

Failure. Coverage is below 100%.

Diff Coverage

chia/cmds/coin_funcs.py (0.0%): Missing lines 7,44-45,109,111,143-144,148,184,188-189,220,223

Total: 318 lines

arvidn left a comment

Choose a reason for hiding this comment

arvidn commented Jul 26, 2023 • edited

jack60612 commented Jul 26, 2023

github-actions bot commented Jul 27, 2023

altendky left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 25, 2023

arvidn left a comment

Choose a reason for hiding this comment

altendky left a comment

Choose a reason for hiding this comment

altendky left a comment

Choose a reason for hiding this comment

arvidn commented Jan 6, 2024

arvidn commented Jan 6, 2024

altendky left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 16, 2024

github-actions bot commented Jan 16, 2024

wjblanke commented Mar 13, 2024

jack60612 commented Jul 9, 2023 •

edited

Diff Coverage
Diff: origin/main...HEAD, staged and unstaged changes

Total: 318 lines
Missing: 67 lines
Coverage: 78%

arvidn commented Jul 26, 2023 •

edited