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

Distribute Asset Market Fees to Referral Program #1419

Merged
merged 17 commits into from Jan 29, 2019

Conversation

Projects
6 participants
@OpenLedgerApp
Copy link
Contributor

commented Nov 6, 2018

  • initial implementation
  • unit tests: added asset_rewards_test, modified create_advanced_uia
Distribute Asset Market Fees to Referral Program
- initial implementation
- unit tests: added asset_rewards_test, modified create_advanced_uia
@pmconrad
Copy link
Contributor

left a comment

Good job, thanks!

Show resolved Hide resolved libraries/chain/db_balance.cpp Outdated
Show resolved Hide resolved libraries/chain/db_market.cpp Outdated
Show resolved Hide resolved libraries/chain/db_market.cpp Outdated
Show resolved Hide resolved libraries/chain/db_market.cpp Outdated
Show resolved Hide resolved libraries/chain/db_market.cpp Outdated
Show resolved Hide resolved libraries/chain/db_balance.cpp Outdated
Show resolved Hide resolved libraries/chain/db_balance.cpp Outdated
Show resolved Hide resolved libraries/chain/db_market.cpp Outdated
Show resolved Hide resolved libraries/chain/db_market.cpp Outdated
Show resolved Hide resolved tests/tests/market_fee_sharing_tests.cpp Outdated

@oxarbitrage oxarbitrage referenced this pull request Nov 8, 2018

Closed

Distribute Asset Market Fees to Referral Program #1268

7 of 11 tasks complete
@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

Peter, thank you for your comments. They're quite useful!
We have updated our PR

Unit tests for asset
- added market_sharing_whitelist option
- added tests for asset extensions before and after the hardfork 1268
@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2018

I forgot one thing: you must add checks to proposal_create_evaluator to prevent asset_create or asset_update from being included into the blockchain before the hardfork. See proposal_operation_hardfork_visitor. Also add unit tests for this.

@abitmore

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

I forgot one thing: you must add checks to proposal_create_evaluator

And need code to avoid unexpected fee schedule update. See 93110cb

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

And need code to avoid unexpected fee schedule update. See 93110cb

In this PR we didn’t introduce any new operations and didn’t change any logic related to fee schedule.
May i kindly ask you to elaborate a bit on your concerns ?

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

I forgot one thing: you must add checks to proposal_create_evaluator ...

Done.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2018

Please add hardfork protection to prevent vesting_balance_create operation with instant_vesting_policy_initializer in

  • vesting_balance_create_evaluator
  • proposal_create_evaluator
  • unit tests for both
@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

Please add hardfork protection to prevent vesting_balance_create operation with instant_vesting_policy_initializer ...

Done

OpenLedgerApp added some commits Nov 27, 2018

Check asset restrictions
- added check asset restrictions while pay reward
- added unit test
@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

why api call get_mfs_vesting_balances and cli wallet call were removed ?

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

why api call get_mfs_vesting_balances and cli wallet call were removed ?

That was a temporary solution. Recently we have introduced serialization for vesting_balance_type. Now it is possible to use get_vesting_balances method to distinguish market fee sharing balance.

@thul3

This comment has been minimized.

Copy link

commented Dec 18, 2018

Great direction but in my opinion there are some small lacks for mass adoption.
The first issue which has been proofen in the past is the LTM.Webmasters don't pay to be able to promote an offer.Thats something which will discourage majority of webmasters to promote bitshares.
The next point is to deliver massiv traffic promoters need to be able to calculate.Since it is a refferal program it means the webmaster needs to wait a long time for his full reward.He needs some kind of guarantee that the reward won't decrease after a short time when Asset owners update their shared market fee.Will the market_fee_reward_percent asset option gives the possibility that refferals which has been already sent will get a lower reward if changed ?
I also think there should be a global consenus about the market fee reward percent since why should one Asset owner take advantage of the traffic another registrar/asset owner is getting for paying a share of 50% from trading fee.
I would also like to ask if using that solution will it be possible for registrars to recruite local webmasters to promote your assets and get a cut of the market fee's earned by their recruited webmasters ?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

Please rebase on latest hardfork branch to resolve conflicts.

@oxarbitrage oxarbitrage added this to In Development in Protocol Upgrade Release (3.0.0) via automation Jan 15, 2019

@MichelSantos

This comment has been minimized.

Copy link

commented Jan 17, 2019

Great direction but in my opinion there are some small lacks for mass adoption.

@thul3 Your comments about promotion are valuable but this isn't the right forum for that discussion because this Github issue is focused on technical implementations for BSIP-43 on Market Fee Sharing rather than on the merits of BSIP-43.

I encourage you to discuss these comments either in the discussion area for BSIP-43 or alternatively consider discussing an alternative or a supplement to BSIP-43 with the community.

OpenLedgerApp added some commits Jan 23, 2019

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

Good for me, thanks!

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

@oxarbitrage I think your comments have been addressed. Please verify and approve if so.

Protocol Upgrade Release (3.0.0) automation moved this from In Development to In Testing Jan 27, 2019

@oxarbitrage
Copy link
Member

left a comment

looks great now, thank you for the hard work.

@oxarbitrage
Copy link
Member

left a comment

hold on, i think i a a problem with the commits history and hardfork date. resulting hardfork date form this pull is the old one:
https://github.com/bitshares/bitshares-core/blob/6b3d6ab81eab813d46a8fd57a4911099199f4036/libraries/chain/hardfork.d/CORE_1268.hf

As there is a rebase that is adding the old date again after that at: 1b9f6c9

please check.

Protocol Upgrade Release (3.0.0) automation moved this from In Testing to In Development Jan 27, 2019

Protocol Upgrade Release (3.0.0) automation moved this from In Development to In Testing Jan 28, 2019

@oxarbitrage
Copy link
Member

left a comment

thank you.

@oxarbitrage oxarbitrage merged commit 8714026 into bitshares:hardfork Jan 29, 2019

1 of 2 checks passed

ci/dockercloud Your tests failed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Protocol Upgrade Release (3.0.0) automation moved this from In Testing to Done Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.