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

Add BTC address param, use String for param value instead of long #1841

Conversation

ManfredKarrer
Copy link
Member

No description provided.

We assumed a BTC price of 10 000 USD and a BSQ rice of 1 USD to get a
90% discount if the user choose the BSQ fee. As BTC price is more in
the 6000 USD range we adjust the BSQ fee to stick with the 90% discount.
Implementation is not complete yet. It is missing the handling for the
not BSQ enabled case and applying to to the take offer screen.
As take offer screen is WIP the fee display is also just a raw draft.
For the DAO state we should add a new group for the fee selection as
it is a bit lost in between the other fields...
- Change type of param value from long to string for more flexibility
- Add BTC recipient address param
- Add validation, refactor param value formatter and param value parser
@ManfredKarrer ManfredKarrer changed the title Make param value string Add BTC address param, use String for param value instead of long Oct 30, 2018
Copy link
Member

@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 - @ManfredKarrer I guess this is mainly to have more flexibility within the DAO?

@ManfredKarrer
Copy link
Member Author

Yes, it make it easier to add new param types (like in that case the btc donation address, which would not have been possible with long data type for the value. In a string u can pack all even byte arrays...

@ManfredKarrer ManfredKarrer merged commit f958ccd into bisq-network:release-candidate-0.9.0 Oct 30, 2018
@ManfredKarrer ManfredKarrer deleted the make-param-value-string branch October 30, 2018 18:17
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

Had a look at the String param changes and added some comments.

@@ -770,7 +773,22 @@ public void setNewParam(int blockHeight, Param param, long paramValue) {
});
}

public long getParamValue(Param param, int blockHeight) {
public Coin getParamValueAsCoin(Param param, int blockHeight) {
Copy link
Member

Choose a reason for hiding this comment

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

These getParamValueAs...() functions could use a check that they're used correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean to check the type? as they are called only from switch cases by type i am not sure if its needed. would throw exceptions otherwise...

}
}

public String formatParamValue(Param param, String value) {
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if these functions could avoid the large switch blocks and use the ParamType now that it's been added. I think it would make sense, might take a look at that later, I think it would be safer.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yes, good idea...

public void validateParamValue(Param param, long paramValue) throws ChangeParamValidationException {
// max 4 times the current value. min 25% of current value as general boundaries
checkMinMax(param, paramValue, 300, -75);
public void validateParamValue(Param param, String inputValue) throws ValidationException {
Copy link
Member

Choose a reason for hiding this comment

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

All these values would be nice to have in Param.java to be able to see all the relevant info for each param. Perhaps ParamType could also store the restrictions?

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 we could add it the params

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

3 participants