Implement BSIP87: Force-Settlement Fee

[christophersanborn]

Implementation of BSIP87.

Mostly complete but need container for debt asset to receive collateral-denominated fees.

• Collateral-asset fee container (via PR Collateral asset fee accumulator for BSIPs 74 and 87 #2159)
• Fee calculation and routing
• Test cases

[abitmore]

IMO the fastest solution is to reuse the container for market fee sharing, although it's not the best. From a product manager's point of view, the container should be bound to the asset but not an account, then we need additional operations for the asset owners to claim the funds which means more development work.

[abitmore]

If we go with the approach that adding a new container into the asset_dynamic_asset_object, I think we can reuse the asset_claim_fees_operation to claim the fees in the container by adding an asset_id field in the extensions.

[abitmore]

The code doesn't compile. It seems some new code didn't got pushed to the repository.

Another thing is we also need to charge fees for instant force-settlements (for GSed assets).

[christophersanborn]

The code doesn't compile. It seems some new code didn't got pushed to the repository.

Correct, it now depends on PR #2159, which is not merged in on this branch. Question: @abitmore — If I rebase this branch on top of hardfork tip, (or, actually, on top of PR #2159, which is based on hardfork tip as of yesterday), will it erase the discussion history here? Or would it be better to "merge" recent hard fork back into this branch?

Another thing is we also need to charge fees for instant force-settlements (for GSed assets).

I'm not sure I agree with this. It's not mentioned in the BSIP — and I note there is something odd about an asset owner charging fees on settlement for an asset that's in a "failed" state, as opposed to letting claimants claim their full value from the settlement pool. It also could be a vector for an exit scam — as the asset owner could in theory ramp the fees up substantially after GS and make off with a big chunk of the pool, effectively abandoning their product and their customers.

I would argue that the differences between "instant settling" and "force settling" are not just semantic but also substantive, and enough so to argue that instant settling is not covered by the BSIP as written. I'm trying not to editorialize too much though.

If I recall, but I haven't gone back to double-check, the market fees mechanism is also not triggered during instant settle, (although perhaps it should be, since they are collected by the collateral issuer, who was not responsible for the GS). This also partly informed my choice not to touch the GS settle code.

[abitmore]

Merging from base branch is usually better for tracking old discussions.

Fees about instant settlement was discussed here: bitshares/bsips#260 (comment). You're correct that it's not explicitly described in the BSIP document. I don't think this PR is the best place to discuss it, will forward your comments to the bsips repo. IMO, when trading an asset, the traders need to trust the issuer/owner to an extent, as a platform we provide tools for both the owners and the traders E.G. asset permissions.

[christophersanborn]

don't think this PR is the best place to discuss it, will forward your comments to the bsips repo.

Cool thanks for raising the point there. I'll consider it a point of active discussion and await resolution there.

[abitmore]

Merging from base branch is usually better for tracking old discussions.

@christophersanborn you merged from the container branch, although the container branch has been merged into the hardfork branch now, it seems Github doesn't render well but still shows the changes in the container branch, which makes it a bit unclear for code review. It's not a big deal though. If you'd like to fix it, you can recreate the branch from the commit before the merge, merge hardfork, then force-push (but not rebase directly. Also remember to make a backup before acting).

[christophersanborn]

Merging from base branch is usually better for tracking old discussions.

@christophersanborn you merged from the container branch, although the container branch has been merged into the hardfork branch now, it seems Github doesn't render well but still shows the changes in the container branch, which makes it a bit unclear for code review. It's not a big deal though. If you'd like to fix it, you can recreate the branch from the commit before the merge, merge hardfork, then force-push (but not rebase directly. Also remember to make a backup before acting).

@abitmore — Cool, thanks for tip. Looks better now. Should be some new commits coming in over next couple of days addressing above review points and such.

[abitmore]

Thanks. If got chance please wrap the long lines (max 118 characters per line).

[abitmore]

Thanks. Code looks good. Expecting test cases.

[abitmore]

Merging for progress.

[christophersanborn]

Thanks. Code looks good. Expecting test cases.

Thanks. I have a number of test cases already thanks to @MichelSantos. I am just making slight adjustments to them due to moving the force_settle_fee_percent into bitasset_opts. Should be able to check them in in the morning.

[christophersanborn]

@abitmore — I've added more commits to christophersanborn:cjs-bsip87, incorporating @MichelSantos's unit tests. But I do not see the update here, presumably because the PR has been merged. What is the right way to proceed? Can this PR be re-opened? Or should I open a new one?

[christophersanborn]

Also, one more question — a thing I want to be sure I am doing right: (@abitmore)

In asset_update_bitasset_evaluator::do_apply(), I’ve made no changes. It looks to me like the existing code basically basically takes the new options and replaces the old options with them. (Along with some preconditions and postcondition trigger checks.) And I am assuming that it is correct to leave this alone.

But If I’m interpreting the code correctly, this means that if any optional parameter is unset (like the new force_settle_fee_percent), then it will be unset after the update is applied… even if that parameter previously had a value.

Is that the correct interpretation of unset optional fields in an update operation? And does that match the way these are treated in other operations?

In other words, I can see two possible interpretations of an unset optional field in an update operation:

(1) I do not wish to change this parameter — after the update, it should retain the value it previously held.

(2) I wish the parameter to be unset after the update — I.e., even if a value was previously set, I want the new state to be unset.

I would be inclined to think that interpretation (1) would be the interpretation “of least surprise”, unless precedent has already established interpretation (2).

Do we need to add interpretation (1) logic to ::do_apply()?

[abitmore]

a) For test cases, please create a new pull request.
b) For asset_update_bitasset_operation, IMO it's better to go interpretation (2) since it keeps the code simple, if we go (1) then we need a complex design to be able to unset the new parameters when they were already set. I do think it's better to document it though. There was a similar discussion for the optional target_collateral_ratio parameter in call_order_update_operation.

[christophersanborn]

@abitmore wrote:

b) For asset_update_bitasset_operation, IMO it's better to go interpretation (2) since it keeps the code simple, if we go (1) then we need a complex design to be able to unset the new parameters when they were already set. I do think it's better to document it though. There was a similar discussion for the optional target_collateral_ratio parameter in call_order_update_operation.

Not sure that this generalizes to all optional parameters, but in case of force_settle_fee_paramter, since unset is interpreted as zero anyway, it is not strictly required to have an option to un-set the parameter.

One possible middle ground would be to reject the operation in do_evaluate if an optional field is unset in the operation but set (and non-zero) in the existing object. That would force the asset owner to be explicit — and I think could have some merit.

I do agree we should not pursue interpretation (1) unless we are doing it across the board — it would be worse to have inconsistent update semantics than to have non-ideal semantics. If we are being consistent with the rest of the API (primary point of comparison would be the existing reward_percent options in additional_asset_options), then interpretation (2) is fine by me.

[abitmore]

Yes, I agree to keep consistent with the rest of code base. In current code base we have quite some places doing interpretation (2), so IMO it's good that we keep it as is.

[abitmore]

@christophersanborn wrote:

primary point of comparison would be the existing reward_percent options in additional_asset_options

According to the code, there is no special code about setting or unsetting the reward_percent parameter in asset options.