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

Collateral asset fee accumulator for BSIPs 74 and 87 #2159

Merged

Conversation

christophersanborn
Copy link
Member

  • Adds field in asset_dynamic_data_object to accumulate fees denominated in a bitasset's collateral backing asset.

  • Extends asset_claim_fees operation to enable claim of collateral-denominated fees.

  • Checks that accumulated collateral fees is empty before allowing change of backing asset.

@abitmore abitmore added this to In development in Protocol Upgrade Release (4.0.0) via automation Apr 28, 2020
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.

We can add a test case about the hard fork time in this PR. More test cases about data can be done in #2130 or #2151 or after one of them is merged.

libraries/protocol/include/graphene/protocol/asset_ops.hpp 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/asset_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/include/graphene/chain/asset_object.hpp Outdated Show resolved Hide resolved
@christophersanborn
Copy link
Member Author

christophersanborn commented May 2, 2020

@abitmore wrote:

We can add a test case about the hard fork time in this PR. More test cases about data can be done in #2130 or #2151 or after one of them is merged.

Would we just be testing that an asset_claim_fees_operation with extensions is rejected pre-hardfork? Where would be best place to put such a test? Perhaps @MichelSantos can include along with BSIP87 and BSIP74 tests.

As an aside: do we ever just make one hardfork symbol, like HARDFORK_CORE_V4_0_0_TIME instead of individual symbols for each feature, each of which presumably anchoring to the same activation date?

@abitmore
Copy link
Member

abitmore commented May 3, 2020

As an aside: do we ever just make one hardfork symbol, like HARDFORK_CORE_V4_0_0_TIME instead of individual symbols for each feature, each of which presumably anchoring to the same activation date?

Using the same macro for different feature has pros and cons. For example, we planned to release 4.0 with BSIP40 enabled, the feature is already in the release branch but is not ready for release yet, we can set its hf time to a date far away in the future so it won't be enabled at the same time.

Protocol Upgrade Release (4.0.0) automation moved this from In development to In testing May 3, 2020
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 a lot!
I'm approving and merging for progress although there are some small issues.

container_ddo->accumulated_fees :
container_ddo->accumulated_collateral_fees),
"Attempt to claim more fees than have accumulated within asset ${a} (${id})",
("a",container_asset->symbol)("id",container_asset->id)("ddo",*container_ddo) );
Copy link
Member

Choose a reason for hiding this comment

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

The code logic is correct. However I think it would be better to change this big and complex assertion to multiple smaller and simpler assertions for better readability and error reporting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ambivalent on this one, but considering. May sneak a rephrasing into BSIP87 PR.

@@ -55,7 +55,16 @@ namespace detail {
"Taker fee percent should not be defined before HARDFORK_BSIP_81_TIME");
}
}
}

void check_asset_claim_fees_hardfork_87_74_collatfee(const fc::time_point_sec& block_time, const asset_claim_fees_operation& op)
Copy link
Member

Choose a reason for hiding this comment

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

Line too long.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix in BSIP87 PR.

@@ -31,6 +31,7 @@ namespace graphene { namespace chain {
namespace detail {
void check_asset_options_hf_1774(const fc::time_point_sec& block_time, const asset_options& options);
void check_asset_options_hf_bsip81(const fc::time_point_sec& block_time, const asset_options& options);
void check_asset_claim_fees_hardfork_87_74_collatfee(const fc::time_point_sec& block_time, const asset_claim_fees_operation& op);
Copy link
Member

Choose a reason for hiding this comment

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

Line too long.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix in BSIP87 PR.

@abitmore abitmore merged commit b6785e0 into bitshares:hardfork May 3, 2020
Protocol Upgrade Release (4.0.0) automation moved this from In testing to Done May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants