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.

Sign up for GitHub

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

Jump to bottom

BSIP81: Simple maker-taker fees #2136

Merged
merged 22 commits into from Apr 22, 2020
Merged

BSIP81: Simple maker-taker fees #2136

merged 22 commits into from Apr 22, 2020

Conversation

MichelSantos
Copy link
Contributor

This initial PR contains 3 tests of BSIP81. At least 4 additional tests still remain to be completed.

This PR might need to be redone onto an alternate branch.

@abitmore abitmore changed the base branch from develop to hardfork April 7, 2020 03:32
@abitmore abitmore added this to In development in Protocol Upgrade Release (4.0.0) via automation Apr 13, 2020
@MichelSantos
Copy link
Contributor Author

It should be noted that there is a possible drawback to this implementation: after the hardfork, if an old client software submits an asset_create_operation, the asset will be created with a taker fee set to 0%. The asset owner can subsequently update the taker fee to something else with an asset_update_operation.

I consider that to be reasonable behavior but it can be altered with additional complexities if there is a strong objection.

@abitmore
Copy link
Member

if an old client software submits an asset_create_operation, the asset will be created with a taker fee set to 0%.

Good catch. IMO the logic should be: if the owner didn't set a taker fee (old clients), then the old market fee rate applies to takers. It's an optional field, so it's not hard to distinguish between not set and 0.

abitmore added a commit to bitshares/bsips that referenced this pull request Apr 15, 2020
@abitmore
Update specification and wording of BSIP 81
f60735c 
It's found that the specification was not clear enough.
See also: bitshares/bitshares-core#2136 (comment)
@abitmore abitmore mentioned this pull request Apr 15, 2020
Merged
@abitmore
Copy link
Member

Created a pull request to update the specification: bitshares/bsips#271.

abitmore
abitmore reviewed Apr 15, 2020
View reviewed changes
libraries/protocol/asset_ops.cpp Outdated Show resolved Hide resolved
libraries/chain/asset_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/asset_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/asset_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/db_maint.cpp Show resolved Hide resolved
libraries/chain/db_market.cpp Outdated Show resolved Hide resolved
libraries/chain/hardfork.d/BSIP_81.hf Outdated Show resolved Hide resolved
@abitmore
Copy link
Member

Thanks. The code looks good so far, but perhaps there are conflicts when merging.
I haven't reviewed the test cases yet. Will do another review after it's labeled "ready for review".

@abitmore
Copy link
Member

@MichelSantos please click "Ready for review"? Or you think it's still not ready for merge?

@dls-cipher dls-cipher marked this pull request as ready for review April 20, 2020 14:04
@dls-cipher
Copy link
Collaborator

dls-cipher commented Apr 20, 2020 •
edited

I confirmed "ready" for review and will follow up with @MichelSantos. If anything is missing, he can push another PR.
@abitmore - please proceed with audit/review

@MichelSantos
Copy link
Contributor Author

@MichelSantos please click "Ready for review"? Or you think it's still not ready for merge?

I was double-checking with some CLI tests. I think that it's ready for review.

abitmore
abitmore reviewed Apr 20, 2020
View reviewed changes
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.

tests/tests/simple_maker_taker_fee_tests.cpp Show resolved Hide resolved
tests/tests/simple_maker_taker_fee_tests.cpp Show resolved Hide resolved
@abitmore abitmore moved this from In development to In testing in Protocol Upgrade Release (4.0.0) Apr 20, 2020
abitmore
abitmore previously approved these changes Apr 22, 2020
View reviewed changes
Protocol Upgrade Release (4.0.0) automation moved this from In testing to In development Apr 22, 2020
abitmore
abitmore approved these changes Apr 22, 2020
View reviewed changes
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.

The CI tests have passed. Looks like github has issues updating the CI status.

Protocol Upgrade Release (4.0.0) automation moved this from In development to In testing Apr 22, 2020
@abitmore
Copy link
Member

Thanks.

@abitmore abitmore merged commit 4bda2d9 into hardfork Apr 22, 2020
Protocol Upgrade Release (4.0.0) automation moved this from In testing to Done Apr 22, 2020
@MichelSantos MichelSantos deleted the bsip81 branch April 22, 2020 23:50
@MichelSantos MichelSantos mentioned this pull request May 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophersanborn christophersanborn christophersanborn left review comments

@winston-1984 winston-1984 winston-1984 left review comments

@abitmore abitmore abitmore approved these changes

Assignees

@MichelSantos MichelSantos

@dls-cipher dls-cipher

Labels
hardfork
Projects
No open projects
Protocol Upgrade Release (4.0.0)
  
Done
Milestone
4.0.0 - Protocol Upgrade Release
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MichelSantos @abitmore @dls-cipher @christophersanborn @winston-1984