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.

Sign up for GitHub

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

Jump to bottom

add dust warning message to chia coins commands & cleanup code #14301

Merged
merged 1 commit into from
Jul 25, 2023
Merged

add dust warning message to chia coins commands & cleanup code #14301

merged 1 commit into from
Jul 25, 2023

Conversation

jack60612
Copy link
Contributor

@jack60612 jack60612 commented Jan 6, 2023 •
edited
Loading

Purpose:
This adds a warning to the user letting them know that the dust filter might prevent them from seeing their coins.

Current Behavior:
No warning when making coins below the dust limit, user might be confused as wallet ignores coins.
New Behavior:
A warning was added to stop possible user confusion.

Testing Notes:
CLI Testing will be added in the coming weeks.

@jack60612 jack60612 added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label Jan 6, 2023
@jack60612 jack60612 requested a review from emlowe January 6, 2023 20:19
@jack60612 jack60612 requested a review from a team as a code owner January 6, 2023 20:19
arvidn
arvidn previously requested changes Jan 9, 2023
View reviewed changes
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 don't think the 3rd commit is safe

chia/cmds/coin_funcs.py Outdated Show resolved Hide resolved
chia/cmds/coin_funcs.py Outdated Show resolved Hide resolved
chia/cmds/coin_funcs.py Outdated Show resolved Hide resolved
@jack60612 jack60612 linked an issue Jan 17, 2023 that may be closed by this pull request
Coin combine command issue when used with CATs and a fee[Bug] #14330
Closed
@jack60612 jack60612 requested a review from arvidn January 18, 2023 17:23
arvidn
arvidn reviewed Jan 18, 2023
View reviewed changes
chia/cmds/coin_funcs.py Outdated Show resolved Hide resolved
chia/cmds/coins.py Outdated Show resolved Hide resolved
chia/cmds/coin_funcs.py Outdated Show resolved Hide resolved
chia/cmds/coin_funcs.py Outdated Show resolved Hide resolved
chia/cmds/coin_funcs.py Outdated Show resolved Hide resolved
chia/cmds/coin_funcs.py Outdated Show resolved Hide resolved
@jack60612 jack60612 changed the title Add dust filter warning to chia coins split Fix chia wallet coins cli bugs & add dust filter warning to chia coins split Jan 22, 2023
@jack60612 jack60612 changed the title Fix chia wallet coins cli bugs & add dust filter warning to chia coins split Fix chia wallet coins cli bugs & add dust filter warning to chia coins split Jan 22, 2023
@jack60612 jack60612 requested a review from arvidn January 22, 2023 18:26
arvidn
arvidn reviewed Jan 23, 2023
View reviewed changes
chia/rpc/wallet_rpc_api.py Outdated Show resolved Hide resolved
arvidn
arvidn reviewed Jan 23, 2023
View reviewed changes
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.

A lot of this code is not controversial, and I would be happy to approve it. But since this PR does so many things (dust filter, cleanup, fixing several bugs), it makes it harder to land the non-controversial parts.

Specifically, the smallest possible changes to fix the existing bugs, I believe would be simple enough to approve and land before we gave tests.

chia/rpc/wallet_rpc_api.py Outdated Show resolved Hide resolved
@jack60612 jack60612 marked this pull request as draft January 23, 2023 17:06
@jack60612 jack60612 changed the title Fix chia wallet coins cli bugs & add dust filter warning to chia coins split Cleanup chia coins commands Jan 23, 2023
@jack60612 jack60612 changed the title Cleanup chia coins commands add dust warning message to chia coins commands Jan 23, 2023
@jack60612 jack60612 force-pushed the jn.add-dust-warning branch 2 times, most recently from daad770 to bdc388a Compare January 23, 2023 19:33
@jack60612 jack60612 changed the title add dust warning message to chia coins commands add dust warning message to chia coins commands & cleanup code Jan 23, 2023
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jan 25, 2023
@github-actions
Copy link
Contributor

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

@emlowe emlowe removed the 1.7.0 label Jan 30, 2023
@github-actions
Copy link
Contributor

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

@github-actions github-actions bot added the stale-pr Flagged as stale and in need of manual review label Mar 17, 2023
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jul 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2023

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

@jack60612 jack60612 removed the stale-pr Flagged as stale and in need of manual review label Jul 8, 2023
@jack60612 jack60612 dismissed arvidn’s stale review July 8, 2023 19:01

no longer relevant

@jack60612 jack60612 requested a review from arvidn July 8, 2023 19:02
@jack60612 jack60612 marked this pull request as ready for review July 8, 2023 19:02
@jack60612 jack60612 requested a review from a team July 8, 2023 19:03
@jack60612 jack60612 added the CLI label Jul 8, 2023
@jack60612 jack60612 requested a review from arvidn July 13, 2023 21:39
arvidn
arvidn approved these changes Jul 25, 2023
View reviewed changes
chia/cmds/coin_funcs.py Show resolved Hide resolved
@wallentx wallentx merged commit 2d7e8ab into main Jul 25, 2023
198 of 199 checks passed
@wallentx wallentx deleted the jn.add-dust-warning branch July 25, 2023 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arvidn arvidn arvidn approved these changes

@emlowe emlowe Awaiting requested review from emlowe

Assignees
No one assigned
Labels
CLI Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Coin combine command issue when used with CATs and a fee[Bug]
4 participants
@jack60612 @arvidn @wallentx @emlowe