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

Fix memo plotid #17856

Merged
merged 7 commits into from Apr 30, 2024
Merged

Fix memo plotid #17856

merged 7 commits into from Apr 30, 2024

Conversation

pmaslana
Copy link
Contributor

Purpose:

Fixes issues where "chia plotters" command is returning bytes and causing type issues. This also removed the bytes32 for the hex arg, which is 128 bytes.

Current Behavior:

New Behavior:

Testing Notes:

Created and duplicated plots using the plot ID and memo using "chia plots create" and "chia plotters chiapos"

@pmaslana pmaslana requested a review from a team as a code owner April 11, 2024 04:13
@pmaslana pmaslana linked an issue Apr 11, 2024 that may be closed by this pull request
Copy link
Contributor

@emlowe emlowe left a comment

Choose a reason for hiding this comment

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

You also need to change the line
plot_memo: bytes32 = stream_plot_info_pk(keys.pool_public_key, keys.farmer_public_key, sk)

and remove the bytes32

plot_memo = stream_plot_info_pk(keys.pool_public_key, keys.farmer_public_key, sk)

I'm also not sure about the bytes or string check - might be better to also fix up the way chia plotters works instead - although this would work

chia/plotting/create_plots.py Outdated Show resolved Hide resolved
@emlowe emlowe added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label Apr 12, 2024
@wjblanke
Copy link
Contributor

If we fix earles comment we can get this PR in.

Copy link
Contributor

File Coverage Missing Lines
chia/plotting/create_plots.py 11.1% lines 214-215, 217-218, 223-224, 226-227
Total Missing Coverage
9 lines 8 lines 11%

Copy link
Contributor

@emlowe emlowe left a comment

Choose a reason for hiding this comment

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

Coverage diff is expected as this didn't have coverage before

@pmaslana pmaslana merged commit ca897f1 into main Apr 30, 2024
350 of 351 checks passed
@pmaslana pmaslana deleted the fix-memo-plotid branch April 30, 2024 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Wrong type with memo argument
3 participants