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

Reputation BSQ added to BSQ Wallet screen #3366

Merged
merged 3 commits into from Nov 26, 2019
Merged

Reputation BSQ added to BSQ Wallet screen #3366

merged 3 commits into from Nov 26, 2019

Conversation

niyid
Copy link
Contributor

@niyid niyid commented Oct 6, 2019

Issue #3352 resolved.

closes #3352

@niyid niyid requested review from ripcurlx and sqrrm as code owners October 6, 2019 17:48
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

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

NACK - if my interpretation is correct.

Copy link
Contributor

@freimair freimair left a comment

Choose a reason for hiding this comment

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

I am not happy with the term "Reputation/Weight Balance". I myself (as an active bisq dev) do not understand what the value is about. Furthermore, one might misunderstand this value for riches available.

A few additional words that might clear things up:

  • "base balance"
  • "base merit"
  • "base vote weight"
  • "base vote weight (without stake)"
  • "for future voting cycles"
  • "for next voting cycle"

and we should align the wording here https://docs.bisq.network/dao/overview.html#reputation-based-voting

@m52go may we have your opinion

@niyid
Copy link
Contributor Author

niyid commented Nov 1, 2019

I am not happy with the term "Reputation/Weight Balance". I myself (as an active bisq dev) do not understand what the value is about. Furthermore, one might misunderstand this value for riches available.

A few additional words that might clear things up:

  • "base balance"
  • "base merit"
  • "base vote weight"
  • "base vote weight (without stake)"
  • "for future voting cycles"
  • "for next voting cycle"

and we should align the wording here https://docs.bisq.network/dao/overview.html#reputation-based-voting

@m52go may we have your opinion

OK. I am still waiting on the consensus on what term to use. @m52go @freimair

I think we should involve much more people on this discussion.

@m52go
Copy link
Contributor

m52go commented Nov 4, 2019

How about something like:

"Reputation Value (not spendable)"

OR

"Reputation Value (more details 🔗)"

If it's possible, I think the second option with a link to the current doc is better. There, this value is called "reputation value" and explains what it is and how it works.

I agree with freimair that calling this quantity a balance is confusing, because it's not spendable.

@niyid
Copy link
Contributor Author

niyid commented Nov 4, 2019

How about something like:

"Reputation Value (not spendable)"

OR

"Reputation Value (more details )"

If it's possible, I think the second option with a link to the current doc is better. There, this value is called "reputation value" and explains what it is and how it works.

I agree with freimair that calling this quantity a balance is confusing, because it's not spendable.

In that case, Reputation should suffice (removing the Balance appendage). Only problem is, @chimp1984 already pointed out that the docs are outdated.

Please refer to the related #3352.

@m52go
Copy link
Contributor

m52go commented Nov 4, 2019

Oh ok, I didn't see that conversation. The doc I referred to is the most up-to-date one, so I guess that needs to be updated.

As for my suggestions, simply replace "Reputation" with "Merit".

@niyid
Copy link
Contributor Author

niyid commented Nov 4, 2019

Oh ok, I didn't see that conversation. The doc I referred to is the most up-to-date one, so I guess that needs to be updated.

As for my suggestions, simply replace "Reputation" with "Merit".

I already have it as this:

dao.reputationBalance=Merit BSQ

@freimair is this OK with you?

Once again, please refer to related issue #3352 and the suggestion by @chimp1984.

@m52go
Copy link
Contributor

m52go commented Nov 4, 2019

I already have it as this:

dao.reputationBalance=Merit BSQ

I'm still seeing this dao.reputationBalance=Reputation/Weight Balance in the PR code.

In any case, I would strongly recommend adding some clarification text in parenthesis as suggested above. Either explicitly say it's not spendable or link to more information.

Concretely:

"Merit Value (not spendable)"

OR

"Merit Value (more details 🔗)"

Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

NACK

@niyid
Copy link
Contributor Author

niyid commented Nov 5, 2019 via email

@niyid niyid requested review from sqrrm and freimair November 6, 2019 08:12
freimair
freimair previously approved these changes Nov 9, 2019
Copy link
Contributor

@freimair freimair left a comment

Choose a reason for hiding this comment

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

Ack. We should squash this contribution because there is quite a lot of commits and merges and stuff.

@niyid stuff we observed with your contributions:

  • please keep the commit history clean (make yourself familiar with eg. git rebase --onto, git add -p
  • please stick to the given code style (in intellij idea: Menu [Code]->[Reformat Code])
  • please only commit stuff that has something to do with the topic of the PR (no import, field definition reorgs, ...)

@niyid
Copy link
Contributor Author

niyid commented Nov 9, 2019

Ack. We should squash this contribution because there is quite a lot of commits and merges and stuff.

@niyid stuff we observed with your contributions:

  • please keep the commit history clean (make yourself familiar with eg. git rebase --onto, git add -p
  • please stick to the given code style (in intellij idea: Menu [Code]->[Reformat Code])
  • please only commit stuff that has something to do with the topic of the PR (no import, field definition reorgs, ...)

Noted. Thanks.

I have squashed the commits.

sqrrm
sqrrm previously approved these changes Nov 11, 2019
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

utACK

Still need ack from @ManfredKarrer for changes in DAO code to be able to merge

@chimp1984
Copy link
Contributor

Still need ack from @ManfredKarrer for changes in DAO code to be able to merge

This only applies to code in the dao package in core not in Desktop.

@niyid
Copy link
Contributor Author

niyid commented Nov 14, 2019 via email

@ripcurlx
Copy link
Contributor

ripcurlx commented Nov 14, 2019

@niyid could you please address the comment from @sqrrm. If that is changed ACK from my side as well, besides the fact that you still need to sign all your commits 😉

@ripcurlx
Copy link
Contributor

Maybe the easiest thing would be to squash your commits into one as the last ones also doesn't fulfill https://chris.beams.io/posts/git-commit.

@niyid
Copy link
Contributor Author

niyid commented Nov 14, 2019

@niyid could you please address the comment from @sqrrm. If that is changed ACK from my side as well, besides the fact that you still need to sign all your commits

Final commit

Screenshot from 2019-11-14 16-24-01

@ripcurlx
Copy link
Contributor

I'm talking about all commits in this PR. Otherwise GitHub will prevent it getting merged.
Bildschirmfoto 2019-11-14 um 16 59 23

@niyid niyid dismissed stale reviews from sqrrm and freimair via 42a732f November 14, 2019 16:14
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

NACK - the formatting in BsqBalanceUtil line 132 is not correct. Please use our formatting rules and also it would be great if you could just fork the bisq repository regularly and allow edits by maintainers. If it would have been like that I'd push the formatting fix myself.

freimair
freimair previously approved these changes Nov 19, 2019
Copy link
Contributor

@freimair freimair left a comment

Choose a reason for hiding this comment

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

Ack

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

NACK - please see my comments

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show reputation balance in BSQ wallet stats
6 participants