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

Dao ui improvements #1751

Merged
merged 31 commits into from Oct 12, 2018

Conversation

ManfredKarrer
Copy link
Member

No description provided.

- Add COMPENSATION_REQUEST_MIN_AMOUNT and COMPENSATION_REQUEST_MAX_AMOUNT
As BSQ based validation values can change over time if BSQ value
rise we need to support that in the Params as well.

- Add validateParamValue to ChangeParamValidator
- Show current formatted value
- Parse input to correct domain value
- Add validation (WIP)
- Add boundary checks (WIP)
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.

I would like some clarification on my comments on how calculations are done. Something seems strange about it, either it's correct and some code comments are needed or something isn't working as intended.

break;
case BLIND_VOTE_FEE:
break;
case COMPENSATION_REQUEST_MIN_AMOUNT:
Copy link
Member

Choose a reason for hiding this comment

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

There should also be a check that COMPENSATION_REQUEST_MIN_AMOUNT < COMPENSATION_REQUEST_MAX_AMOUNT, both current and proposed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes the validations are very much WIP...

@VisibleForTesting
long getNewValueByPercentChange(Param param, long percentChange) {
checkArgument(percentChange > -100, "percentChange must be bigger than -100");
return (getCurrentValue(param) * 100 * (100 + percentChange)) / 100;
Copy link
Member

Choose a reason for hiding this comment

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

This multiplies by 100 one time too many.

@ManfredKarrer
Copy link
Member Author

The validation code for params is WIP and I am not sure if the current implementation is intuitive (range set by % of current value...). I will keep that for a bit later and try to get other high prio stuff done first. But feel free to work on that if you want.

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

Some comments on parts that might need more documentation but please go ahead and merge.

ObservableList<BlindVotePayload> appendOnlyStoreList = blindVoteListService.getAppendOnlyStoreList();
appendOnlyStoreList
.forEach(blindVotePayload -> {
// We want a random delay between 0.1 and 30 sec. depending on the number of items
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to depend on the number of items?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't want to stress the network with many broadcasts, so if there are many objects it will be delayed, the more objects the longer the dealy, but not more then 30 sec.

@ManfredKarrer ManfredKarrer merged commit 1163ceb into bisq-network:master Oct 12, 2018
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.

None yet

2 participants