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

More hardfork cleanup #1751

Merged
merged 25 commits into from Oct 19, 2019
Merged

More hardfork cleanup #1751

merged 25 commits into from Oct 19, 2019

Conversation

jmjatlanta
Copy link
Contributor

This is a continuation of #1743

Finishes that ticket, and cleans hardforks from 3.0.1

@abitmore abitmore added this to In development in Feature Release (3.2.0) via automation May 8, 2019
@abitmore abitmore added this to the 3.2.0 - Feature Release milestone May 8, 2019
abitmore
abitmore reviewed May 8, 2019
View reviewed changes
libraries/chain/asset_evaluator.cpp Show resolved Hide resolved
libraries/chain/asset_evaluator.cpp Show resolved Hide resolved
libraries/chain/db_maint.cpp Outdated Show resolved Hide resolved
libraries/chain/market_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/market_evaluator.cpp Outdated Show resolved Hide resolved
tests/tests/market_fee_sharing_tests.cpp Outdated Show resolved Hide resolved
@jmjatlanta jmjatlanta marked this pull request as ready for review May 13, 2019 20:32
@pmconrad
Copy link
Contributor

chain_test failing in travis

abitmore
abitmore reviewed May 16, 2019
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.

By the way,

  • I think FC_ASSERT( reward < issuer_fees, "Market reward should be less than issuer fees"); is a bug, should be <=. Fixing this requires a hard fork.
    • obj.accumulated_fees += issuer_fees.amount - reward.amount; is unnecessary when the amounts are identical.
  • in deposit_market_fee_vesting_balance(seller.registrar, registrar_reward);, registrar_reward can be zero.

libraries/chain/db_market.cpp Outdated Show resolved Hide resolved
@abitmore abitmore mentioned this pull request May 25, 2019
Closed
17 tasks
@pmconrad pmconrad added this to In development in Feature Release (3.3.0) via automation Jun 19, 2019
@pmconrad pmconrad removed this from In development in Feature Release (3.2.0) Jun 19, 2019
@jmjatlanta jmjatlanta added the 2e Ready for Testing Status indicating the solution is sufficiently developed to begin testing label Jul 4, 2019
@abitmore
Copy link
Member

abitmore commented Jul 5, 2019

@jmjatlanta Can you rebase and remove these 2 commits from history?

  • Remove parts of 1465
  • Revert "Remove parts of 1465"

Best if squash the last commit into the 2nd last commit as well.

Thanks.

@jmjatlanta jmjatlanta force-pushed the jmj_1743 branch 3 times, most recently from f8ac4dc to 718098c Compare July 9, 2019 19:12
@ryanRfox ryanRfox moved this from In development to In testing in Feature Release (3.3.0) Jul 16, 2019
@pmconrad
Copy link
Contributor

Your excel sheet is undecided about core-583.
Also, core-588 and core-604 are marked as 'Probably', please check and remove if possible.

@jmjatlanta jmjatlanta added 2d Developing Status indicating currently designing and developing a solution and removed 2e Ready for Testing Status indicating the solution is sufficiently developed to begin testing labels Aug 2, 2019
@jmjatlanta jmjatlanta removed this from In testing in Feature Release (3.3.0) Aug 7, 2019
@jmjatlanta jmjatlanta added this to In development in Protocol Upgrade Release (4.0.0) via automation Aug 7, 2019
@jmjatlanta
Copy link
Contributor Author

I updated the spreadsheet and code for 583, 588, and 604.

@jmjatlanta jmjatlanta added 2e Ready for Testing Status indicating the solution is sufficiently developed to begin testing and removed 2d Developing Status indicating currently designing and developing a solution labels Aug 7, 2019
@MichelSantos MichelSantos mentioned this pull request Aug 9, 2019
Closed
@pmconrad
Copy link
Contributor

Snapshot comparison @38m blocks is good.

Protocol Upgrade Release (5.0.0) automation moved this from In testing to In development Sep 16, 2019
@ryanRfox ryanRfox moved this from In development to In testing in Protocol Upgrade Release (5.0.0) Oct 15, 2019
@ryanRfox ryanRfox added this to In development in Protocol Upgrade Release (4.0.0) via automation Oct 15, 2019
@ryanRfox ryanRfox moved this from In development to In testing in Protocol Upgrade Release (4.0.0) Oct 15, 2019
@ryanRfox ryanRfox removed this from In testing in Protocol Upgrade Release (5.0.0) Oct 15, 2019
pmconrad
pmconrad approved these changes Oct 19, 2019
View reviewed changes
Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

I think all comments have been addressed. Approving so we can merge it into the upcoming release.
Any remaining cleanups can be done in a new PR.

@pmconrad pmconrad merged commit acef99b into develop Oct 19, 2019
Protocol Upgrade Release (4.0.0) automation moved this from In testing to Done Oct 19, 2019
@pmconrad pmconrad deleted the jmj_1743 branch October 19, 2019 08:26
@pmconrad pmconrad mentioned this pull request Oct 19, 2019
Closed
17 tasks
@MichelSantos MichelSantos mentioned this pull request Oct 19, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abitmore abitmore abitmore left review comments

@pmconrad pmconrad pmconrad approved these changes

Assignees
No one assigned
Labels
2e Ready for Testing Status indicating the solution is sufficiently developed to begin testing
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
4 participants
@jmjatlanta @pmconrad @abitmore @ryanRfox