Adding Taker fee percent support#3419
Adding Taker fee percent support#3419AmmarYousefM wants to merge 89 commits intobitshares:developfrom
Conversation
|
How exactly are the machanics here? a) In general, |
| reward_percent: createObject.reward_percent * 100 || 0, | ||
| whitelist_market_fee_sharing: [] | ||
| whitelist_market_fee_sharing: [], | ||
| taker_fee_percent: createObject.taker_fee_percent * 100 || 0 |
There was a problem hiding this comment.
What's the meaning of the || operator here?
If taker_fee_percent is not set (left blank or there is a checkbox to set it but unchecked?), it should be set to null (which means to use market_fee_percent) but not 0 (which means the fee is zero). Think: how to NOT set it?
There was a problem hiding this comment.
I'd copied the same logic of reward_percent; hence not sure if reward_percent logic is correct.
I know that when switching off the entire market fee options; the operation will unset all to null; I don't think with existing UI we can unset those extensions individually; perhaps @sschiessl-bcp can help here.
There was a problem hiding this comment.
There is no problem for reward_percent because the default value is 0 if reward_percent is null. I'm not sure if it can be copied to taker_fee_percent since the logic is different. Waiting for @sschiessl-bcp.
There was a problem hiding this comment.
This is an unwanted opinion of the UI, if not set, it should be undefined and the default logic of core should be used, whatever that may be.
Please be aware that currently also a user set value of 0 would default to undefined then, but that may be wrong. User setting 0 is different from user wanting to set default.
| extensions.whitelist_market_fee_sharing = auths.whitelist_market_fee_sharing.toJS(); | ||
| } | ||
|
|
||
| if (update.taker_fee_percent !== undefined) { |
There was a problem hiding this comment.
The logic here is suspicious. Think: how to remove/unset taker fee from an asset so that both the maker and taker fee would be decided by market_fee_percent?
There was a problem hiding this comment.
It looks fine. Assuming we fix that taker_fee_percent is undefined if not set, then this is the correct
There was a problem hiding this comment.
Just FWIW, in bitshares-core, if an optional value in an asset's asset_options.extensions (E.G. taker_fee_percent) was not undefined, and if in the asset_update_operation it is set to undefined, the behavior is NOT to keep the old value unchanged, but to set it to undefined.
| "reward_percent": "Reward percent", | ||
| "reward_percent_tooltip": "If a market fee is set, the reward percent indicates how much of that market fee is shared through the referral rewards program instead of only benefitting the asset owner.", | ||
| "taker_fee_percent": "Taker Fee percent", | ||
| "taker_fee_percent_tooltip": "If taker fee is set, the taker percent indicates how much percent is shared with the asset owner.", |
There was a problem hiding this comment.
I agree; will use same descriptions in https://github.com/bitshares/bsips/blob/master/bsip-0081.md
| "issuer": "Asset owner", | ||
| "market_fee": "Market fee", | ||
| "market_fee_referral_reward_percent": "Market fee referral reward", | ||
| "market_fee_referral_taker_fee_percent": "Taker fee percent", |
There was a problem hiding this comment.
I don't think it is a good variable or label name, it is misleading.
Add a public api
sschiessl-bcp
force-pushed
the
taker_fee
branch
from
49b50a9 to
1a4aed3
Compare
|
Rebased to #3450 |
6 participants
Options order:
Example:
