Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CHIA-1730: port chia plotnft to @chia_commands framework #18833

Merged
merged 62 commits into from
Dec 3, 2024

Conversation

emlowe
Copy link
Contributor

@emlowe emlowe commented Nov 7, 2024

  • Port chia plotnft to the @chia_commands framework
  • Add many tests to cover chia plotnft flows
  • Changed many error conditions to raise an exception that click handles. The error text will display and importantly click will return an "error" exit code. Previously the code printed out some text and just returned. This caused a success exit code
  • Added some code to handle earlier cases where users use a wallet ID that is not a pooling wallet. Previous code would error out much later in the process. Now this error is caught upfront before asking the user to continue. Also added an early check if there a no pool wallets period.
  • Added some code to handle cases where the wallet ID is not given. In this case, if there is one and only one pool wallet, that wallet will be selected automatically. This makes it easier on the user since they don't always have to specify the wallet id in the simple case of having a single pool wallet. An error will be returned in this case if there are multiple pool wallets. When there are multiple pool wallets the user does need to specify the wallet by id.
  • The exception to the above behaviour is when using the plotnft show command. In order to keep backwards behaviour when no wallet_id is provided all pool wallets will be used and basic details about them displayed. This should work identically to previous versions
  • Also added in fixes to plotnft code that always generated new puzzlehashes (3 places) - this fix is somewhat unrelated but was surfaced when enabling the reuse parameterization

@emlowe emlowe added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Nov 7, 2024
Copy link

coveralls-official bot commented Nov 7, 2024

Pull Request Test Coverage Report for Build 12036886833

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 575 of 580 (99.14%) changed or added relevant lines in 9 files are covered.
  • 23 unchanged lines in 11 files lost coverage.
  • Overall coverage increased (+0.3%) to 91.461%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/_tests/conftest.py 39 44 88.64%
Files with Coverage Reduction New Missed Lines %
chia/util/full_block_utils.py 1 98.17%
chia/_tests/simulation/test_simulation.py 1 96.45%
chia/cmds/plotnft.py 1 98.91%
chia/_tests/pools/test_pool_wallet.py 1 99.04%
chia/server/address_manager.py 1 91.89%
chia/consensus/make_sub_epoch_summary.py 1 98.8%
chia/timelord/timelord.py 2 78.58%
chia/full_node/full_node.py 2 86.51%
chia/_tests/util/full_sync.py 3 88.07%
chia/wallet/wallet_node.py 4 88.18%
Totals Coverage Status
Change from base Build 12036226533: 0.3%
Covered Lines: 104162
Relevant Lines: 113709

💛 - Coveralls

chia/cmds/plotnft.py Outdated Show resolved Hide resolved
@emlowe emlowe changed the title chia plotnft CLI improvements CHIA-1730: chia plotnft CLI improvements Nov 8, 2024
Copy link
Contributor

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

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Nov 12, 2024
chia/_tests/pools/conftest.py Outdated Show resolved Hide resolved
chia/_tests/cmds/test_cmd_framework.py Show resolved Hide resolved
chia/_tests/pools/test_pool_cli_parsing.py Outdated Show resolved Hide resolved
chia/_tests/pools/test_pool_cli_parsing.py Show resolved Hide resolved
chia/cmds/plotnft.py Outdated Show resolved Hide resolved
chia/cmds/plotnft.py Outdated Show resolved Hide resolved
chia/cmds/plotnft.py Show resolved Hide resolved
chia/cmds/plotnft_funcs.py Outdated Show resolved Hide resolved
chia/cmds/plotnft_funcs.py Outdated Show resolved Hide resolved
chia/cmds/plotnft_funcs.py Outdated Show resolved Hide resolved
chia/cmds/plotnft.py Outdated Show resolved Hide resolved
Copy link
Contributor

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

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Nov 21, 2024
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Nov 22, 2024
Copy link
Contributor

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

Copy link
Contributor

File Coverage Missing Lines
chia/_tests/conftest.py 88.6% lines 1326-1327, 1329-1330, 1333
Total Missing Coverage
580 lines 5 lines 99%

chia/cmds/plotnft_funcs.py Show resolved Hide resolved
chia/cmds/plotnft_funcs.py Show resolved Hide resolved
chia/cmds/plotnft_funcs.py Show resolved Hide resolved
@emlowe emlowe requested a review from Quexington December 2, 2024 17:47
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should aim to have much smaller PRs than this in the future

@pmaslana pmaslana merged commit 7c01e3c into main Dec 3, 2024
364 of 365 checks passed
@pmaslana pmaslana deleted the EL.plotnft-cli-id branch December 3, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants