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

Implement BSIP87: Force-Settlement Fee #2151

Merged
merged 8 commits into from
May 7, 2020

Conversation

christophersanborn
Copy link
Member

@christophersanborn christophersanborn commented Apr 21, 2020

Implementation of BSIP87.

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

@abitmore
Copy link
Member

abitmore commented Apr 22, 2020

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.

libraries/chain/db_market.cpp Show resolved Hide resolved
libraries/chain/db_market.cpp 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/asset_evaluator.cpp Outdated Show resolved Hide resolved
@abitmore
Copy link
Member

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 abitmore marked this pull request as draft April 22, 2020 14:28
libraries/chain/db_market.cpp Outdated Show resolved Hide resolved
libraries/chain/db_market.cpp Show resolved Hide resolved
@abitmore
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

abitmore commented May 3, 2020

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
Copy link
Member Author

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
Copy link
Member

abitmore commented May 4, 2020

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

@abitmore abitmore marked this pull request as ready for review May 7, 2020 22:48
Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Code looks good. Expecting test cases.

@abitmore
Copy link
Member

abitmore commented May 7, 2020

Merging for progress.

@abitmore abitmore merged commit fde2aa4 into bitshares:hardfork May 7, 2020
@christophersanborn
Copy link
Member Author

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
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link
Member

abitmore commented May 9, 2020

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
Copy link
Member Author

christophersanborn commented May 9, 2020

@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.

@christophersanborn christophersanborn mentioned this pull request May 9, 2020
@abitmore
Copy link
Member

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
Copy link
Member

abitmore commented May 12, 2020

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants