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

Remove analytics operator bond requirement #3232

Merged
merged 1 commit into from
Sep 9, 2019
Merged

Remove analytics operator bond requirement #3232

merged 1 commit into from
Sep 9, 2019

Conversation

wiz
Copy link
Contributor

@wiz wiz commented Sep 9, 2019

@wiz wiz requested a review from ripcurlx as a code owner September 9, 2019 14:12
chimp1984
chimp1984 reviewed Sep 9, 2019
View reviewed changes
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

Changing those values was never tested well in the DAO and its pretty complex to test.

@wiz
Copy link
Contributor Author

wiz commented Sep 9, 2019

@chimp1984 since the analytics operator role hasn't been released in any version of Bisq yet, it should be safe 😅

@ripcurlx
Copy link
Contributor

ripcurlx commented Sep 9, 2019

NACK

Changing those values was never tested well in the DAO and its pretty complex to test.

utACK - As long as it wasn't used nor released I think it should be quite safe to change.

ripcurlx
ripcurlx approved these changes Sep 9, 2019
View reviewed changes
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.

utACK

@ripcurlx ripcurlx merged commit 322fe23 into bisq-network:master Sep 9, 2019
@wiz wiz deleted the increase-analytics-operator-bond-amount branch September 9, 2019 21:50
@chimp1984
Copy link
Contributor

Please revert! It is risky to change that even if it was not used. Breaking DAO consensus can have severe effects and that change does not justify the risk.

@sqrrm
Copy link
Member

sqrrm commented Sep 10, 2019

I have to agree with @chimp1984 on this one. It seems like a very small benefit compared to the risk of breaking the DAO consensus.

@wiz wiz changed the title Increase analytics operator bond amount from 1 to 2 Remove analytics operator bond requirement Sep 10, 2019
@wiz
Copy link
Contributor Author

wiz commented Sep 10, 2019

@m52go yeah I spoke with the Chimp and he said that while the changes we made are probably fine, even if there's a 0.1% risk of breaking the DAO, we shouldn't do it without huge testing, which is a valid point, so we're going to revert the changes and remove the bond requirement for now.

@ripcurlx ripcurlx mentioned this pull request Oct 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chimp1984 chimp1984 chimp1984 left review comments

@ripcurlx ripcurlx ripcurlx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wiz @ripcurlx @chimp1984 @sqrrm