bsip#43 Market fee sharing #107

OpenLedgerDev · 2018-08-29T09:50:12Z

PR for #102

According to the comments: #102 (comment)

What is the reasoning to have a seperate "asset_claim_reward_operation", and not funnel it through existing referral reward mechnism?

A separate account_reward_object and asset_claim_reward_operation are needed to track profits from "market fee sharing".
Vesting_balance_object receives fee from all transactions.

The market_sharing_whitelist allows discrimination. Is there a viable use-case for this?

The market_sharing_whitelist is an additional functionality. Asset Issuer may not use this list.

Agreed to the points above, making this a general dividend for holding the asset would be misleading the intent

We have read a few times and we're not sure we understand points 2 and 3 correctly. Could you elaborate a bit more please?

xeroc · 2018-09-05T06:34:33Z

Nice specs!

Instead of setting up a new object account_reward_object, I would also like to understand why not to reuse the infrastructure of vesting balances.

Also, I would actually think that there is more use for a black list than for a whitelist. Why not have both?

OpenLedgerApp · 2018-09-05T15:00:21Z

Instead of setting up a new object account_reward_object, I would also like to understand why not to reuse the infrastructure of vesting balances.

Vesting_balance_object receives fees from all kinds of transactions.
In this case, a user can't find out how much profit is received from market fee sharing.
So it is better to create a new object account_reward_object in order to track profits from market fee sharing only.

Also, I would actually think that there is more use for a black list than for a whitelist. Why not have both?

That's a very good point! However, considering there are only a few referrers, black list seems like excessive functionality. It's one or the other, there is no point using both.
We think that instead of blacklisting, it's safer to whitelist registrars that we trust specifically.
@xeroc What do you think?

pmconrad · 2018-09-05T16:36:40Z

bsip-0043.md

+**claim_reward(account, asset_symbol, amount)** - Claim account's reward for the given asset.
+
+## account_reward_object
+A new BitShares object type in implementation space impl_account_reward_object_type = 2.18.X that tracks the rewards of a single account/asset pair


Please consider using this mechanism for all kinds of rewards. Related: bitshares/bitshares-core#1276
(Perhaps I misunderstand what you mean with "tracks" here - are you talking about rewards payment history?)

Yes, you're right. this could be a potential solution!
However, the related bitshares/bitshares-core#1276 is not implemented yet.
So one way would be to implement the way that is described in the proposed BSIP.
And the other way would be to Implement #1276 first. Then, we can use this mechanism to implement part of BSIP-43.
Our team can definitely implement #1276 - providing that estimation and budget are approved first.
If that's approved - we are totally up to changing bsip to use the newly implementing #1276 features.

pmconrad · 2018-09-05T16:40:58Z

bsip-0043.md

+[asset_type, reward_amount desc, owner_account]
+```
+## asset_claim_reward_operation
+A new operation used to transfer reward to the account's balance.


Please explain why this is required.
It would be much simpler to either pay these rewards automatically, e. g. during maintenance like the current rewards system does, or to use the existing vesting balance mechanism.

concerning vesting balance mechanism, please see our response to xeroc above and to your comment above.
And paying out directly during maintenance, if we understand correctly, there is no such mechanism right now. In maintenenance, only process_fees is called, that distributes accumulated fees to the network, registrar and rererrer using deposit_cashback (the vesting balance mechanism we discussed above).

see our response to xeroc above and to abit below.

Sorry I didn't see a new response so far.

... if we understand correctly, there is no such mechanism right now. In maintenenance, only process_fees is called ...

Since we're discussing how to implement a new feature, I think adding more tasks to be done in maintenance interval IS an option.

Sorry I didn't see a new response so far.

sorry, @abitmore we planned to respond to your commment below, but i think we covered most of it to the response to Peter's comment above :)
Denis

Since we're discussing how to implement a new feature, I think adding more tasks to be done in maintenance interval IS an option.

Direct transfer would be quite an alternative mechanism to vesting balance.
Yes, we can transfer directly, however there has to be a reason why fees distribution happens through vesting balance.
Do you guys know exact reasons why this was was implemented?
We have some ideas, but we're not 100% sure.

So we want to replicate the current functionality (or use the current vesting balance mechanism as discussed above).
Direct transfer to the normal balance is an option, however, there has to be a solid reason to do that.
We can definitely put it up to discussion. I suggest to create a separate comment/thread for this topic . Because it's quite a drastic change and we want to make sure the reasoning behind this is quite solid.

Denis

why fees distribution happens through vesting balance.

cashback is designed to have a vesting period;

for better performance for non-maintenance blocks (only process fees on maintenance blocks). Updating one object v.s. updating 3 objects and related looking-ups for the objects. Arguable though.

Guys, we had another discussion today about direct transfer. So we think having direct transfer does not generate separate transactions and hence does not generate network fee.
So we think that the best way would be to recreate vesting-like mechanism - so that we utilize a similar mechanism to the current mechanism of claiming accumulated fees from assets (but for referrers and registrars. ) Then they have to use claim - to claim their accumulated fees and hence, generate network fees for bitshares.

Alternatively, as we mentioned, using current vesting mechanism would be a great idea, but we then need to discuss the estimation and priority for #1276. Should we start this discussion?
What do you think?
Denis

Sorry, I didn't get it. Which is better in your opinion, "generate network fees", or not? I foresee that reward claiming fee won't be any big part of overall transaction fee. It's even possible that the committee will set balance claiming fee to zero since it removes data from RAM.

From a technical point of view, having to claim means more signed transaction on the chain, which will lead to chain bloating to an extent (more permanent storage); having to maintain unclaimed balances means larger chain state (more RAM usage). On the other hand, seems it requires less computation (less CPU usage).

In regards to user experience (UX) and business development (BD), each approach has its own pros and cons.

It's all about compromise.

I want to add that it also makes a differences for taxes.

In germany, vesting balance is an "Option", whereas a direct payout would have direct tax implications.

Ergo, pro vesting!

bsip-0043.md

pmconrad · 2018-09-05T16:52:26Z

bsip-0043.md

+
+# Abstract
+
+When creating a new asset, the asset issuer is the only beneficiary of the market fees in the current implementation. And the only way to affect the asset community growth is the market fee percentage. For example, one can decrease the market fee and it will result in somewhat larger number of trades with this asset. In this way the asset issuer might get a bigger profit during to increasing the trade volume with this asset.


Certainly not "the only way". I doubt that global BTC trade volume depends on market fees alone.

You're right, it's not the only way, but one of the ways

pmconrad · 2018-09-05T16:53:22Z

bsip-0043.md

+The workflow may be as following:
+
+An asset issuer defines the market_fee_reward_percent - what percentage of the market fee he wants to share with the registrar in asset options. Registrar defines reward_percent for the referrer for each user(using the already existing BitShares mechanism).
+Market fee reward is accumulated on the user account. The user decides when they want to claim the market fee reward and move it to their active balance. There is another operation created asset_claim_reward_operation. Each user pays network fee to call this operation.


Why so complicated?

It's not more complicated than vesting balance mechanism. In fact, the idea is copied from the vesting balance mechanism and then we simplified to some extent.

pmconrad · 2018-09-05T16:53:56Z

bsip-0043.md

+Percent of market fee that will be paid to buyer's registrar. Set by UIA issuer.
+
+## market_sharing_whitelist asset option
+An optional list of accounts (configurable by the UIA Issuer) could provide increased control over who is eligible to get market rewards.


~~UIA issuer~~ asset owner

These terms are interchangeable in our understanding.
For example if we look in the code: in the general case, issuer = owner. It is the same account.

class asset_object : public graphene::db::abstract_object<asset_object>{ ... /// ID of the account which issued this asset. account_id_type issuer; ... };

There does not seem to be owner field. It's called issuer.
If the current widely accepted way would be to use the term "asset owner" - we can definitely change it everywhere in the document.
Do you think we should change to asset owner

I guess Peter's opinion was that it's inappropriate to use UIA here, since the option can be used on MPAs as well.

The term "issuer" is somewhat misleading. Although the field is called "issuer" internally, the "issuer" of an MPA can't issue tokens. Owner is more appropriate IMO.
(And yes, the proposed feature is not limited to UIAs alone.)

I think we can switch to the asset owner term, no problem

pmconrad · 2018-09-05T16:54:31Z

bsip-0043.md

+
+## New database and wallet api methods
+**get_account_reward(account, asset_symbol)** - Returns information about the reward amount for specific asset.
+**list_account_rewards(account)** - Returns information about the reward amount in various assets


database api calls should have start and limit parameters

We think that one user will generally have only a few assets with rewards. So we don't think pagination is needed in this particular case. On one hand, it's definitely good to follow the common standards.
On the other hand, in bitshares there are other well-known functions that do not have pagination, for example:
list_account_balances(account_id)

Alternatively, we can change list_account_rewards to get_account_rewards.
Or if following standards is becoming a trend in bitshares, we can add start and limit parameters as you suggested.
What do you think?

Makes sense. I don't particularly care, can leave it as is.

pmconrad · 2018-09-05T16:55:54Z

bsip-0043.md

+indexed_by
+[id]
+[owner_account, asset_type]
+[asset_type, reward_amount desc, owner_account]


Please consider moving indices that are not generally useful or consensus-related into optional plugins.

We do not think that these indices belong to a plugin, because it's definitely consensus-related.
However, if we decide to implement #1276, and then use vesting balance mechanism then we would not these indices anymore.

TBH I think this is over-detailed to be in a BSIP document. It's implementation details, which can be decided by the developer.

TBH I think this is over-detailed to be in a BSIP document. It's implementation details, which can be decided by the developer.

Agreed

Perhaps you're right! We have already half-implemented this BSIP when we did a prototype as part of the investigation if it could be done at all.
So this is already described and to take it out would mean to spend more time :)

The BSIP should not tie you down, and allow to switch if anything would need to be restructured.

I'd recomment putting in an abstract representation of what you want in it, not the actual representation. Will also facilitate understanding it

pmconrad · 2018-09-05T16:57:06Z

bsip-0043.md

+**adjust_reward(account, delta)** - Adjust a particular account's reward in a given asset by a delta
+
+## graphene::chain::database modifications
+**pay_market_fees(seller, recv_asset_object, receives_amount)** - Split pay to asset issuer fee, registrar fee and referrer fee. If the registrar is not in the market_sharing_whitelist, split_pay will not happen and the entire fee goes to Asset issuer.


Please specify behaviour if the new options are absent, or if the whitelist is empty.

Also, note that fee is not paid out the the asset issuer but collected in the asset's fee pool (current behaviour).

if market_fee_reward_percent = 0 or absent - the old mechanism is used. Market fees are accumulated in the asset and can be claimed.

If whitelist empty or absent - there is no filtering. That is like everyone is in the whitelist by default if it is empty.

Also, note that fee is not paid out the the asset issuer but collected in the asset's fee pool (current behaviour).

Yes, this mechanism stays the same way. The fees are accumulated, then claimed, not transferred. However, now not the whole market fee goes to the asset fee pool, but only a part of it: market_fee*(1 - market_fee_reward_percent). And market_fee* market_fee_reward_percent will get to the registar's fee pool .

Thanks, I assumed as much. But you have to update the BSIP accordingly. Comments here are not part of the specification. :-)

abitmore · 2018-09-05T23:47:34Z

Vesting_balance_object receives fees from all kinds of transactions.
In this case, a user can't find out how much profit is received from market fee sharing.
So it is better to create a new object account_reward_object in order to track profits from market fee sharing only.

I think the approach (adding new objects and operations for market fee sharing only) is over-complicated. In order to distinguish a deposit is a market fee sharing or another (including witness pay, worker pay, trx fee sharing and etc), we can add a field in vesting_balance_object to indicate its source, and/or add fields in the virtual deposit operations, which IMHO is better for expanding further in the future.

OpenLedgerApp · 2018-09-07T13:26:52Z

@abitmore We discussed these issues in the first comment of Peter Conrad.
We can definitely implement it using vesting balance mechanism (having done #1276 first).

sschiessl-bcp · 2018-09-28T19:38:42Z

Are the aboves reviews all included/handled in the PR?

OpenLedgerApp · 2018-10-02T14:45:07Z

Are the aboves reviews all included/handled in the PR?

I will add the discussed points.

sschiessl-bcp · 2018-10-02T20:10:27Z

Ping me when dealt with past reviews.

OpenLedgerApp · 2018-10-05T17:19:32Z

We Updated the BSIP-43 according to the discussions with community members.
Denis.
@sschiessl-bcp please have a look.

pmconrad · 2018-10-06T12:46:46Z

Thanks, looks good to me.
But I would really prefer if bitshares/bitshares-core#1276 was implemented first and the generic mechanism be used here as well.

abitmore · 2018-10-12T22:52:59Z

Looks good to me so far, and I'm inline with Peter's preference.

Need to resolve conflicts again.

xeroc · 2018-10-16T11:33:22Z

I think the approach (adding new objects and operations for market fee sharing only) is over-complicated. In order to distinguish a deposit is a market fee sharing or another (including witness pay, worker pay, trx fee sharing and etc), we can add a field in vesting_balance_object to indicate its source, and/or add fields in the virtual deposit operations, which IMHO is better for expanding further in the future.

For cashback, the account_object has an attribute that links the cashback-related vesting balance. Every account could get their own "market fee share cashback" attribute which links to the other vesting balance ... No one says we need to combine them into one vesting balance

OpenLedgerApp · 2018-10-16T12:57:12Z

For cashback, the account_object has an attribute that links the cashback-related vesting balance. Every account could get their own "market fee share cashback" attribute which links to the other vesting balance ... No one says we need to combine them into one vesting balance

We think this is a great idea. Then #1276 becomes not required for this approach.
We can update the document.
@abitmore @pmconrad please voice your opinions as soon as you can so that we can update the BSIP.

Also, guys, do we still need #1276? We're doing the estimations right now, so we need to know if we should proceed with estimations.

Denis

pmconrad · 2018-10-16T13:10:42Z

Should work IMO.

abitmore · 2018-10-17T00:27:45Z

Every account could get their own "market fee share cashback" attribute which links to the other vesting balance ... No one says we need to combine them into one vesting balance

I disagree the approach of storing IDs of vesting balances in account object, because there would be many assets thus many vesting balance objects for each account that need to be linked. The performance would be impacted. IMHO it's better to query vesting balance index directly by new indices. An additional field indicating the source would be necessary for depositing.

IMHO bitshares/bitshares-core#1276 is independent. This PR is about how to process funds. bitshares/bitshares-core#1276 is about how to track history.

OpenLedgerApp · 2018-10-17T14:28:39Z

I disagree the approach of storing IDs of vesting balances in account object, because there would be many assets thus many vesting balance objects for each account that need to be linked. The performance would be impacted. IMHO it's better to query vesting balance index directly by new indices. An additional field indicating the source would be necessary for depositing.

Ok, we can take this approach as well. @pmconrad @xeroc what do you think of abit's idea?

pmconrad · 2018-10-17T14:34:56Z

IMO that's an implementation detail, we don't need to agree on a final solution here and now.
Unsure which is better, both solutions have advantages and disadvantages.

What should be specified is when and how new vesting_balances are created, since vesting_balance_ids are relevant to the protocol (vesting_balance_withdraw requires it).

xeroc · 2018-10-17T19:59:34Z

I agree with @abitmore . havent thought about market fee potentially being many different assets.

OpenLedgerApp · 2018-10-18T16:28:08Z

Guys, based on the discussion above here’s the way we suggest to update BSIP:

There should be a field called Source added into the vesting_balance _object. This field should indicate what kind of vesting balance it is.
There are the following balances in BitShares currently: worker, cashback. We will add another type: Market fee sharing .
Each asset will have its own vesting_balance.
This implementation will allow to track each asset balance while not affecting performance much.

It is important to note that the new field affects the database and we will need to do node replay.
Where as with the reward_object there will be no need to do node replay as we do not change the DB.

We assume that everyone agrees with this way of updating BSIP.
Please let us know if we still need to discuss anything or if anyone still wants to add his opinion.
Thanks, Denis

abitmore · 2018-10-18T22:27:16Z

Just a note: besides of "worker, cashback" and the new market fee sharing vesting balances, vesting balances can be created manually via vesting_balance_create_operation, so please make sure those won't mess up things.

bsip-0043.md

abitmore · 2018-10-23T19:41:13Z

bsip-0043.md

@@ -16,11 +16,11 @@ When creating a new asset, the asset owner is the only beneficiary of the market
 However, there might be another opportunity to promote the asset and stimulate the trading - use native Bitshares referral program. At this time unfortunately an assets owner is not able to share market fees with registrars and referrers to stimulate them to market the asset trading, so we suggest to add this possibility. Furthermore, enabling this feature for MPAs (e.g. bitCNY or bitUSD) can provide additional bounty for Bitshares registrars and referrals which can lead to more traders joining to the ecosystem.

 An asset owner defines the **market_fee_reward_percent**  in asset options - what percentage of the market fee he wants to share with the registrar.
-**market_fee** * **market_fee_reward_percent** goes the registar's fee pool.
+**market_fee** * **market_fee_reward_percent** goes the registrar.


Goes to (I don't know if my grammar is correct though)?

bsip#43 Market fee sharing

d673e66

pmconrad suggested changes Sep 5, 2018

View reviewed changes

OpenLedgerApp mentioned this pull request Oct 2, 2018

BSIP-0043: Market fee sharing #102

Closed

Updated according to the discussions with community members

d84c734

pmconrad approved these changes Oct 6, 2018

View reviewed changes

Merge branch 'master' into bsip43-market-fee-sharing

a0b9dc4

Merge branch 'master' into bsip43-market-fee-sharing

7bcffbd

rewards issue

b9a2774

abitmore reviewed Oct 19, 2018

View reviewed changes

bsip-0043.md Show resolved Hide resolved

updated wording

298a9a9

abitmore reviewed Oct 23, 2018

View reviewed changes

ryanRfox merged commit 440f16a into bitshares:master Oct 24, 2018


		# Abstract

		When creating a new asset, the asset issuer is the only beneficiary of the market fees in the current implementation. And the only way to affect the asset community growth is the market fee percentage. For example, one can decrease the market fee and it will result in somewhat larger number of trades with this asset. In this way the asset issuer might get a bigger profit during to increasing the trade volume with this asset.

bsip#43 Market fee sharing #107

bsip#43 Market fee sharing #107

Conversation

OpenLedgerDev commented Aug 29, 2018

xeroc commented Sep 5, 2018

OpenLedgerApp commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OpenLedgerApp Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschiessl-bcp Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OpenLedgerApp Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschiessl-bcp Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore commented Sep 5, 2018

OpenLedgerApp commented Sep 7, 2018

sschiessl-bcp commented Sep 28, 2018

OpenLedgerApp commented Oct 2, 2018

sschiessl-bcp commented Oct 2, 2018

OpenLedgerApp commented Oct 5, 2018 • edited Loading

pmconrad commented Oct 6, 2018

abitmore commented Oct 12, 2018

xeroc commented Oct 16, 2018

OpenLedgerApp commented Oct 16, 2018 • edited Loading

pmconrad commented Oct 16, 2018

abitmore commented Oct 17, 2018

OpenLedgerApp commented Oct 17, 2018

pmconrad commented Oct 17, 2018

xeroc commented Oct 17, 2018

OpenLedgerApp commented Oct 18, 2018

abitmore commented Oct 18, 2018

Choose a reason for hiding this comment

OpenLedgerApp Sep 7, 2018 •

edited

Loading

sschiessl-bcp Oct 8, 2018 •

edited

Loading

OpenLedgerApp Sep 7, 2018 •

edited

Loading

sschiessl-bcp Oct 8, 2018 •

edited

Loading

OpenLedgerApp commented Oct 5, 2018 •

edited

Loading

OpenLedgerApp commented Oct 16, 2018 •

edited

Loading