added new asset permission flags pay_fees_core_only and exchange_restricted

alexpmorris commented 7 years ago

https://github.com/alexpmorris/bitshares-core/commits/pay_fees_core_only

I added code for two new asset permission flags:

pay_fees_core_only   = 0x200,  /** < require asset-related fees to be paid in core asset only (ie. BTS) */
exchange_restricted  = 0x400  /**< only the issuer can offer asset for sale on exchange */

pay_fees_core_only is especially useful for 0 precision assets, such as those that have been created by @officialfuzzy and others to be used as STEEMIT tokens. Many times, when new bitshares users are given these tokens they do not yet fully understand the system, and often do not have any BTS available to use as fees. As such, they end up using WHALESHARE, HAIRSHARE, BEYONDBIT, etc in lieu of BTS as transfer fees. Since these assets are indivisible, the fee costs multiples of the .24 BTS fee that is generally charged. Having this optional flag set by the asset issuer forces users of these tokens to pay associated fees in BTS, and thus avoids the bitterness caused by dramatically overpaying on their first (or first few) transfers before they realize the mistake they are making.
Yesterday in the telegram chat Bill Butler wrote the following:

What I'd really like to do is create tokens and then offer them for sale on the exchange in tranches. 1000 for $.10 1000 for $.50 1000 for $1.00 And disallow trading while the "ICO" is underway. This effectively eliminates competition from scalpers while I'm raising money. Otherwise, I'll have people buying them for $.10 and turning around to sell them for $.40 or $.90 to undercut the next tranches. The cool part about this is that I could actually post the entire sell order book immediately so my ICO was transparent to all. Then, once all tokens have been sold, I've raised my money and I can lift the vesting limitation.
By adding exchange_restricted, I believe it solves what he'd like to do by only allowing the issuer to sell the asset on the market (post on the book) while the flag is active for that asset. Furthermore, he can limit transfer of the asset by also enabling the transfer_restricted flag. The comment for transfer_restricted implied it may have been intended to support a similar feature ( ... or market orders), however it doesn't seem that aspect was ever implemented (as far as I could tell):

/// @return true if this asset may only be transferred to/from the issuer or market orders
bool is_transfer_restricted()const { return options.flags & transfer_restricted; }

I think both features would be valuable to add because they serve to promote more liquidity by enticing potential ICO / token issuers with more options, and also by helping prevent newer users from overpaying fees and thus having second thoughts over using the platform. If these additions are accepted, it should be relatively trivial to add additional toggles into the GUI to enable/disable these features.

assertion triggered by pay_fees_core_only:

fee_asset->get_id() == asset_id_type(): Fees for asset WHALEHOLE must be paid in BTS
{"asset":"WHALEHOLE","core":"BTS"}
th_a  evaluator.cpp:77 prepare_fee

assertion created by exchange_restricted:

_sell_asset->issuer == _seller->get_id(): Only issuer seller can sell WHALEHOLE on market
{"issuer":"seller","asset":"WHALEHOLE"}
th_a  market_evaluator.cpp:68 do_evaluate

oxarbitrage commented 7 years ago

this does not seems to be an issue but an actual pull request. @alexpmorris in order for us to review and test you need to send the code changes in the form of pull request to the develop branch of bitshares-core.

pmconrad commented 7 years ago

Such changes must be documented and discussed in the form of a BSIP first, IMO.

alexpmorris commented 7 years ago

@xeroc also added on telegram:

I would like to extend this slightly and instead of a boolean flag introduce a 'fee-scheme' for an asset ... with 0: default, 1:BTS only, 2: percentage ... etc ..

There already is market_fee_percentage in asset_options, so perhaps he means something a bit different? I could always add another uint16_t to asset_options instead of the proposed flag. Also, there is the potential issue of the transfer_restricted "market orders" that doesn't seem to have been implemented either (if this is deemed to be an "issue").

Should I go ahead and try issuing this as a pull-request as @oxarbitrage suggested, or hold off for now for something else (ie. BSIP, etc)?

pmconrad commented 7 years ago

Your changes require a hardfork. That means you cannot simply go ahead and change the logic in the code to suit your needs. The code must be able to process the existing blockchain with the currently implemented logic, and at some point (the "hardfork") switch to the new logic.

In addition, you propose a change to the core protocol. BitShares is not just a piece of open source software, it is a distributed organization. Changes to the protocol must be approved by the shareholders. In order to get approval, you must specify why you want to change this, how you want to do it, what the effects on other stakeholders are, etc.. This should be done in the form of a BSIP, see https://github.com/bitshares/bsips/blob/master/bsip-0001.md . Once you have documented this, shareholders can discuss your proposal, and perhaps request modifications. If you think a sufficient number of shareholders are happy with your proposal, you can request formal approval by creating a worker proposal in the blockchain and asking people to vote on it. Only after your proposal has received formal approval (i. e. the worker has been voted in) a pull request will be considered.

alexpmorris commented 7 years ago

That means you cannot simply go ahead and change the logic in the code to suit your needs. The code must be able to process the existing blockchain with the currently implemented logic, and at some point (the "hardfork") switch to the new logic.

These changes should not interfere with the currently implemented logic or functionality, unless you're seeing something that I've missed in the way I implemented it. As for BSIPs, worker proposals, etc, I'm not asking for funding, approvals, or anything.

Just as with the rounding issue, I saw something I believed I could solve (and that I believe would be beneficial to the community to do so), and just went ahead and did it, as opposed to waiting for some "approval" that might never come. In fact, having a potential solution should make it much easier for the "distributed organization" to determine the potential ramifications of the new code, versus hypothesizing and extrapolating what "could" be a problem with a solution that has yet to be implemented, or if those working on it are even capable of performing the task at hand.

If the "distributed organization" feels it's worth their time to consider, great. If not, that's fine too.

It's also why I didn't immediately submit this as a "pull request", as @oxarbitrage had suggested.

pmconrad commented 7 years ago

These changes should not interfere with the currently implemented logic or functionality, unless you're seeing something that I've missed in the way I implemented it.

You define flags that didn't exist before. That is a protocol change. We're running a blockchain here, so you can change the protocol only if everybody agrees to that change.

Suppose some of the witnesses are running your code and some aren't. Those who aren't might accept transactions into their blocks that would be rejected by those who are. Depending on which group is in the majority, you either knock out the fraction running the new code, or you cause missed blocks in the fraction running the old code. In either case you miss a lot of blocks, which threatens the stability of the chain.

I appreciate your enthusiasm and your ideas, but I think someone who is rather new to the project should try their skills on less delicate parts of the code first.

alexpmorris commented 7 years ago

Everything in bitshares-core is a "delicate part of the code", because changing any part could potentially be a breaking change to something else. Even changing the text of a status message that someone might be keying in on and expecting to remain consistent through a hard fork could potentially cause an issue. I get it. Again, it's also why I didn't initially put this in as a "pull-request", because I was proposing it, as indicated in my original message:

I think both features would be valuable to add because they serve to promote more liquidity by enticing potential ICO / token issuers with more options, and also by helping prevent newer users from overpaying fees and thus having second thoughts over using the platform. If these additions are accepted, it should be relatively trivial to add additional toggles into the GUI to enable/disable these features.

But you're acting as if I'm trying to rush something through here, when all I did is make it easier by presenting a potential solution to issues which I laid out above. I understand that all the witnesses have to agree to implement it for an upcoming hardfork for the new functionality to work (and to verify old functionality remains unimpaired). Having a potential "solution" ready for scrutiny should only make that decision easier, not more difficult.

Or, just like the rounding issue, perhaps we can let another three years go by in discussions while I try tackling "less delicate parts of the code", while people continue to complain about the strange fills they receive. At least now there is finally a potential solution on the table for review.

Regardless, I've tackled all I'm willing to pursue regarding the bitshares code for now, so I leave it up to the community to decide if it's of use to them or not.

oxarbitrage commented 7 years ago

i agree with @pmconrad in the fact that changes that require hardfork are different. we have 3 pull requests with hardfork to merge and we still didn't made it. we have 1 big pull with BSIP018(https://github.com/bitshares/bitshares-core/pull/340) and 2 more with small changes.

if you want to make changes that requires hardfork definitely is going to be hard. the same goes with any fee changes, the chain is in production and we have to be more carefull with those changes and also have shareholders approval, it's how it works.

adding code into the bitshares-core is a hard task, everything need to be tested and discussed and some code will be discarded. it is frustrating at the beginning as everything takes a lot more time but it is actually a good thing, gives an important level of security and efficiency to the code added.

keep code and conversation here short, send pull requests to the develop branch with short and precise description of the exact problem your code is fixing or trying to do, make sure that the pull request only have the changes you are working on and nothing more.

it is "easier" to go after changes that don't require hardfork or protocol modifications, api calls, new/edit plugins, wallet, test cases are some examples of where code additions are more likely to be considered.

it is how it works, i didn't invented but it is showing to be an effective system, every programmer that starts sending code to bitshares-core will have to adapt those rules.

xeroc commented 7 years ago

I like the progress, but as described by @pmconrad and @oxarbitrage, the proposed change requires a hard fork and thus shareholder approval. My personal impression is that approvals for non-major changes like this shouldn't be much of a problem. But it needs a BSIP for shareholders to vet the idea easily. Ultimately, even this change will require a 0-pay worker to be voted for and to get sufficient eyes looking into it, a BSIP is highly recommended.

As for the proposal it self. I like the idea of only allowing the issuer to sell shares in the markets and to implement this as a simple flag. We should have that.

As for the other flag that forces people to pay the fee in BTS, I am not sure this makes a lot of sense. If you want to prevent people from using the fee pool, you can simply drain the fee pool to 0 which will prevent people from using your native asset to pay a fee. Since the fee doesn't go to you anyways, it shouldn't matter to you anyways. The selection of the fee asset is a client-side thing currently.

The proposal I made for the fee is to have an asset-specific setting for fees that allows users of an asset (transfer, trade, ..) to follow the asset-specific rules for paying fees, such as:

pay only in CORE
pay a flat fee
pay a percentage fee
... .. again, if the fee pool is empty, the fee cannot be paid in your native asset.

alexpmorris commented 7 years ago

.. again, if the fee pool is empty, the fee cannot be paid in your native asset.

@xeroc , I asked Fuzzy about this and he said he's aware of that aspect, however he didn't know if it's possible to drain funds from the asset "fee pool", as opposed to just waiting until it drains itself. Do you know how that can be done? If there's a way he can empty the fee pool himself, that would definitely negate the issue right off the bat.

abitmore commented 7 years ago

@alexpmorris it's discussed here: https://github.com/bitshares/bitshares-core/issues/188

pmconrad commented 5 years ago

We have an asset_claim_pool_operation now to "withdraw" funds from the fee pool. I believe this makes the pay_fees_core_only flag superfluous.

The exchange_restricted feature may already be achievable using black/whitelisting. Ideas anyone?

abitmore commented 5 years ago

The exchange_restricted feature mentioned in this issue has been implemented with this PR https://github.com/bitshares/bitshares-core/pull/1611, pending BSIP https://github.com/bitshares/bsips/issues/148. Black/whitelisting doesn't resolve it.

bitshares / bitshares-core

added new asset permission flags pay_fees_core_only and exchange_restricted #380