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

Adds message when setting 0 txfee #848

Closed

Conversation

Projects
None yet
4 participants
@Carbon-Zero
Copy link

Carbon-Zero commented Mar 26, 2019

Adds informative message when setting txfee to 0 in RPC console.

Ref Warrows comment in issue #796

So that's where there is space for improvement. But I think a simple message saying that "0 fees are not always possible and the wallet will do its best to achieve it" is the way to go here.

Adds message when setting 0 txfee
Adds informative message when setting txfee to 0 in RPC console.
@rocket-pig

This comment has been minimized.

Copy link

rocket-pig commented Mar 27, 2019

#819 (exact same change?)

@Carbon-Zero

This comment has been minimized.

Copy link
Author

Carbon-Zero commented Mar 27, 2019

No. The message on the commit states that it's invalid, which it is not. Although the code throws an error, the value that is returned is 0. It will send a 0 fee tx if it's possible. At least as I understand it.

@JSKitty

This comment has been minimized.

Copy link

JSKitty commented Mar 27, 2019

No. The message on the commit states that it's invalid, which it is not. Although the code throws an error, the value that is returned is 0. It will send a 0 fee tx if it's possible. At least as I understand it.

My PR allowed maintainers to edit the commits within it, instead of editing it accordingly, it was NACK'd as a whole.

Exact same concept, different text, which could've been edited in my PR by the PIVX Maintainers. Makes no sense to create a second.

@Carbon-Zero

This comment has been minimized.

Copy link
Author

Carbon-Zero commented Mar 27, 2019

No. The message on the commit states that it's invalid, which it is not. Although the code throws an error, the value that is returned is 0. It will send a 0 fee tx if it's possible. At least as I understand it.

My PR allowed maintainers to edit the commits within it, instead of editing it accordingly, it was NACK'd as a whole.

Exact same concept, different text, which could've been edited in my PR by the PIVX Maintainers. Makes no sense to create a second.

Sorry, dude. I don't do the ACKs/nACKs - I suspect these guys are way to busy to be editing code submitted by other people. The point of doing things this way is for you to contribute. This means a couple of things:

  1. Don't put multiple unrelated changes into a single PR. I don't know of a project that would accept that from an outside contributor. Make it easy for the busy folk.
  2. Get it right. It's either merged or not merged. You can't just throw up any message text, even if it's wrong and expect it to be merged.

Don't sweat it. There was almost no code at all in this commit. Not a big deal. If someone can give you credit for it, that's fine with me. That's not why I'm participating.

@JSKitty

This comment has been minimized.

Copy link

JSKitty commented Mar 27, 2019

No. The message on the commit states that it's invalid, which it is not. Although the code throws an error, the value that is returned is 0. It will send a 0 fee tx if it's possible. At least as I understand it.

My PR allowed maintainers to edit the commits within it, instead of editing it accordingly, it was NACK'd as a whole.
Exact same concept, different text, which could've been edited in my PR by the PIVX Maintainers. Makes no sense to create a second.

Sorry, dude. I don't do the ACKs/nACKs - I suspect these guys are way to busy to be editing code submitted by other people. The point of doing things this way is for you to contribute. This means a couple of things:

1. Don't put multiple unrelated changes into a single PR.  I don't know of a project that would accept that from an outside contributor. Make it easy for the busy folk.

2. Get it right.  It's either merged or not merged.  You can't just throw up any message text, even if it's wrong and expect it to be merged.

Don't sweat it. There was almost no code at all in this commit. Not a big deal. If someone can give you credit for it, that's fine with me. That's not why I'm participating.

  1. I'm still getting used to Github, atleast I am trying to contribute.

  2. Regardless, I allowed maintainers to edit, code was right there for them. Was it right? Merge. Was it wrong? Edit it. It was a simple line of plain-text they could've edited, can't tell me they don't have the extra 20 secs to change that. Nevermind creating a whole new PR for it with new text.

@rocket-pig

This comment has been minimized.

Copy link

rocket-pig commented Mar 27, 2019

..not "too busy" (sigh you know how insanely overused that line is??) to talk circles around a suggestion, however. On both my original suggestion (CLI allow zero fee, or at least be honest about fee thats being used before you charge the user) and now on JSKitty's pull.

@Carbon-Zero

This comment has been minimized.

Copy link
Author

Carbon-Zero commented Mar 27, 2019

I'm not trying to speak for them. I don't even know them. Just offering some possibilities as to why your PR was nACKed. They are the reasons I would have done it. I just thought it might help you with some insight. Of course, I could be wrong and this PR could be unacceptable for yet some other reason - and there can be many - and we don't know them all.

@Warrows

This comment has been minimized.

Copy link
Collaborator

Warrows commented Mar 27, 2019

This change is no better than #819. We don't want to error out when trying to set fees to zero.
What I suggested in #796 is a message but without error. And what Fuzzbawls suggested before in #819 (and would be preferable but is more work) is to

adjust how the existing spend RPC functions handle a zero-fee situation

which could for example involve returning a detailed error if the fees are set to zero but for whatever reason it's not possible to craft such a transaction.

As for why we didn't make the change in #819 (this is only my opinion):

  • I feel it's the responsibility of a PR author to improve upon it from whatever information he/she gets.
  • I think it's disempowering to take upon the PR of someone else without explicit request from its author.
  • This issue is low priority as it only concerns advanced users (RPC) and is more of an informational problem as it stands now. And the preffered fix involves quite some work.
@Carbon-Zero

This comment has been minimized.

Copy link
Author

Carbon-Zero commented Mar 27, 2019

So you like it better when settxfee 0 responds with "true"? I was under the impression that this was the problem.

@Carbon-Zero

This comment has been minimized.

Copy link
Author

Carbon-Zero commented Mar 27, 2019

Closing this cuz Warrows don't like it.

And because it's so incredibly insignificant.

@rocket-pig

This comment has been minimized.

Copy link

rocket-pig commented Apr 1, 2019

there's multiple times people have referenced the 'core team' creating new PRs to address same issues outsiders bring up. Why are you all so...not outsider friendly? It's a bad look. You know how much work I and JSKitty have done to contribute to the PIVX cause? You know how much this type of behavior makes me want to continue contributing?

@Warrows

This comment has been minimized.

Copy link
Collaborator

Warrows commented Apr 1, 2019

@rocket-pig are you aware that @Carbon-Zero is not core? I think this fact makes your first point moot. Unless you say that because you think we should have taken over the issue. But I have detailed above why we haven't done so.
As for why we are "not outsider friendly" I'd like to know to what you are refering to specifically? The same about "this type of behavior". Because I've only been trying to be communicating and show the way for you guys to give the best contribution you can. If that's not the way you feel, I'm sorry and we can have a talk in discord if you see ways to improve.
I know that contributing to this github can be harsh in the beginning because rules are scatered in several places and some are just habits.

By the way, no one ever closed #819. And the NACK should be seen as an invitation to improve by looking at the problem under an other angle. That's why it's so detailed.
Also @Carbon-Zero I never said it was "insignificant". Low priority means we are not looking at it right now. It doesn't mean we don't want people to look at it.

@rocket-pig

This comment has been minimized.

Copy link

rocket-pig commented Apr 1, 2019

Warrows you're a great communicator as has been said before. Its an important part of the collab-development thing to do so. Just a thanks for doing it. The lack of communication or the defacto 'core devs are too busy' response to constructive criticism must be remedied and you're doing that, so yeah 👍

@Carbon-Zero

This comment has been minimized.

Copy link
Author

Carbon-Zero commented Apr 2, 2019

To be clear, I think it was I who said "the core devs are too busy". And as Warrows stated, I am not on the dev team, nor did I intend to give anyone that impression. But I see how it could be taken that way.

I only closed the PR that I opened that was similar to yours because I don't plan on working on it. What I submitted was a very low effort fix and I don't consider it important enough to spend more time on. But if you feel differently of course, update your changes to fit the desired result.

I have absolutely no authority here (or anywhere basically).

@Carbon-Zero

This comment has been minimized.

Copy link
Author

Carbon-Zero commented Apr 2, 2019

@Warrows I never said that you said that it wasn't significant.

I said I wasn't going to continue working on it because I think it's incredibly insignificant. It was just part of my reasoning for closing it.

Jeeeeesus everyone. Take a breath. Don't read between my lines because you'll never see anything there. You're lucky if you see stuff on the lines.

@Warrows - if you're drinking coffee you should consider stopping.

@Warrows

This comment has been minimized.

Copy link
Collaborator

Warrows commented Apr 2, 2019

@Carbon-Zero No worries, I'm breathing, no coffee. That's how I understood it at first but rocket-pig comment made me see it under an other light 😅 So I had to be sure to be clear 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.