-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Include plotNFT rewards in get_farmed_amount #19245
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
Include plotNFT rewards in get_farmed_amount #19245
Conversation
It's been that way since the first pools support. My guess is that the idea was those rewards go to the pool, so you shouldn't show them because someone might get confused why the farmed amount isn't in their wallet? But probably no-one has a real good recollection on this |
This will probably break people's scripts if they depend on it. I wouldn't be against a RPC/CLI option to include these numbers, if you are willing to change it to that it would get my approval. |
@wjblanke sure! The current What are the proper "in-the-code" names we have that differentiate ye olde plots from the newer ones? This PR as it is currently, seems to display rewards that include OG rewards + plotNFT plot rewards. For ME, the difference between the |
I believe the suggestion is as follows:
|
I ended up going with |
b082d0d
to
d365037
Compare
Pull Request Test Coverage Report for Build 14713451425Warning: 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
💛 - Coveralls |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Will can u fight through these import renames. I promise to get it merged after this. Thanks |
Yep, had a baby exactly 1 month ago, and have hardly had time to touch a keyboard since. Will get to this. |
b5b9c19
to
13ed3a6
Compare
@wjblanke I managed to get working tests, and worked out all the conflicts. |
@wjblanke codecov failing for lack of codecov on a test file 😵💫 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aok! for main
Approved for merge despite coverage! |
Purpose:
chia get farm summary
currently only returns values for OG plots.Since the majority of people have long since switched to plotNFT plots, I'm not sure why pooling wallets are being excluded from this calculation.
Current Behavior:
chia rpc wallet get_farmed_amount
chia farm summary
New Behavior:
chia rpc wallet get_farmed_amount
chia farm summary
Testing Notes:
I don't know if the changed calculations here would result in some negative behavior elsewhere, like in the GUI.