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
65 changes: 55 additions & 10 deletions libraries/chain/asset_evaluator.cpp
Expand Up @@ -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.

{
// HF_REMOVABLE: Following hardfork check should be removable after hardfork date passes:
FC_ASSERT( !op.extensions.value.claim_from_asset_id.valid() ||
block_time >= HARDFORK_CORE_BSIP_87_74_COLLATFEE_TIME,
"Collateral-denominated fees are not yet active and therefore cannot be claimed." );
}

} // graphene::chain::detail

void_result asset_create_evaluator::do_evaluate( const asset_create_operation& op )
{ try {
Expand Down Expand Up @@ -455,6 +464,9 @@ void_result asset_update_bitasset_evaluator::do_evaluate(const asset_update_bita
FC_ASSERT( asset_obj.dynamic_asset_data_id(d).current_supply == 0,
"Cannot update a bitasset if there is already a current supply." );

FC_ASSERT( asset_obj.dynamic_asset_data_id(d).accumulated_collateral_fees == 0,
"Must claim collateral-denominated fees before changing backing asset." );

const asset_object& new_backing_asset = op.new_options.short_backing_asset(d); // check if the asset exists

if( after_hf_core_922_931 )
Expand Down Expand Up @@ -920,25 +932,58 @@ void_result asset_publish_feeds_evaluator::do_apply(const asset_publish_feed_ope
} FC_CAPTURE_AND_RETHROW((o)) }



/***
* @brief evaluator for asset_claim_fees operation
*
* Checks that we are able to claim fees denominated in asset Y (the amount_to_claim asset),
* from some container asset X which is presumed to have accumulated the fees we wish to claim.
* The container asset is either explicitly named in the extensions, or else assumed as the same
* asset as the amount_to_claim asset. Evaluation fails if either (a) operation issuer is not
* the same as the container_asset issuer, or (b) container_asset has no fee bucket for
* amount_to_claim asset, or (c) accumulated fees are insufficient to cover amount claimed.
*/
void_result asset_claim_fees_evaluator::do_evaluate( const asset_claim_fees_operation& o )
{ try {
FC_ASSERT( o.amount_to_claim.asset_id(db()).issuer == o.issuer, "Asset fees may only be claimed by the issuer" );
const database& d = db();

detail::check_asset_claim_fees_hardfork_87_74_collatfee(d.head_block_time(), o); // HF_REMOVABLE

container_asset = o.extensions.value.claim_from_asset_id.valid() ?
&(*o.extensions.value.claim_from_asset_id)(d) : &o.amount_to_claim.asset_id(d);

FC_ASSERT( container_asset->issuer == o.issuer, "Asset fees may only be claimed by the issuer" );
FC_ASSERT( container_asset->can_accumulate_fee(d,o.amount_to_claim),
"Asset ${a} (${id}) is not backed by asset (${fid}) and does not hold it as fees.",
("a",container_asset->symbol)("id",container_asset->id)("fid",o.amount_to_claim.asset_id) );

container_ddo = &container_asset->dynamic_asset_data_id(d);

FC_ASSERT( o.amount_to_claim.amount <= ((container_asset->get_id() == o.amount_to_claim.asset_id) ?
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.


return void_result();
} FC_CAPTURE_AND_RETHROW( (o) ) }


/***
* @brief apply asset_claim_fees operation
*/
void_result asset_claim_fees_evaluator::do_apply( const asset_claim_fees_operation& o )
{ try {
database& d = db();

const asset_object& a = o.amount_to_claim.asset_id(d);
const asset_dynamic_data_object& addo = a.dynamic_asset_data_id(d);
FC_ASSERT( o.amount_to_claim.amount <= addo.accumulated_fees, "Attempt to claim more fees than have accumulated", ("addo",addo) );

d.modify( addo, [&]( asset_dynamic_data_object& _addo ) {
_addo.accumulated_fees -= o.amount_to_claim.amount;
});
if ( container_asset->get_id() == o.amount_to_claim.asset_id ) {
d.modify( *container_ddo, [&o]( asset_dynamic_data_object& _addo ) {
_addo.accumulated_fees -= o.amount_to_claim.amount;
});
} else {
d.modify( *container_ddo, [&o]( asset_dynamic_data_object& _addo ) {
_addo.accumulated_collateral_fees -= o.amount_to_claim.amount;
});
}

d.adjust_balance( o.issuer, o.amount_to_claim );

Expand Down
2 changes: 1 addition & 1 deletion libraries/chain/asset_object.cpp
Expand Up @@ -178,7 +178,7 @@ string asset_object::amount_to_string(share_type amount) const
}

FC_REFLECT_DERIVED_NO_TYPENAME( graphene::chain::asset_dynamic_data_object, (graphene::db::object),
(current_supply)(confidential_supply)(accumulated_fees)(fee_pool) )
(current_supply)(confidential_supply)(accumulated_fees)(accumulated_collateral_fees)(fee_pool) )

FC_REFLECT_DERIVED_NO_TYPENAME( graphene::chain::asset_bitasset_data_object, (graphene::db::object),
(asset_id)
Expand Down
7 changes: 7 additions & 0 deletions libraries/chain/hardfork.d/CORE_BSIP_87_74_COLLATFEE.hf
@@ -0,0 +1,7 @@
// This hardfork enables the extension to asset_claim_fees_operation to claim collateral-denominated fees.
// These fees are collected by BSIPs 87 and 74. This should be set to match the earlier of either
// HARDFORK_CORE_BSIP87_TIME or HARDFORK_CORE_BSIP74_TIME.
// This hardfork check should be removable after the hardfork date passes.
#ifndef HARDFORK_CORE_BSIP_87_74_COLLATFEE_TIME
#define HARDFORK_CORE_BSIP_87_74_COLLATFEE_TIME (fc::time_point_sec( 1679955066 ) ) // Temporary date until actual hardfork date is set
#endif
3 changes: 3 additions & 0 deletions libraries/chain/include/graphene/chain/asset_evaluator.hpp
Expand Up @@ -166,6 +166,9 @@ namespace graphene { namespace chain {

void_result do_evaluate( const asset_claim_fees_operation& o );
void_result do_apply( const asset_claim_fees_operation& o );

const asset_object* container_asset = nullptr;
const asset_dynamic_data_object* container_ddo = nullptr;
};

class asset_claim_pool_evaluator : public evaluator<asset_claim_pool_evaluator>
Expand Down
41 changes: 41 additions & 0 deletions libraries/chain/include/graphene/chain/asset_object.hpp
Expand Up @@ -65,6 +65,7 @@ namespace graphene { namespace chain {
share_type current_supply;
share_type confidential_supply; ///< total asset held in confidential balances
share_type accumulated_fees; ///< fees accumulate to be paid out over time
share_type accumulated_collateral_fees; ///< accumulated collateral-denominated fees (for bitassets)
abitmore marked this conversation as resolved.
Show resolved Hide resolved
share_type fee_pool; ///< in core asset
};

Expand Down Expand Up @@ -164,6 +165,46 @@ namespace graphene { namespace chain {
template<class DB>
share_type reserved( const DB& db )const
{ return options.max_supply - dynamic_data(db).current_supply; }

/// @return true if asset can accumulate fees in the given denomination
template<class DB>
bool can_accumulate_fee(const DB& db, const asset& fee) const {
return (( fee.asset_id == get_id() ) ||
( is_market_issued() && fee.asset_id == bitasset_data(db).options.short_backing_asset ));
}

/***
* @brief receive a fee asset to accrue in dynamic_data object
*
* Asset owners define various fees (market fees, force-settle fees, etc.) to be
* collected for the asset owners. These fees are typically denominated in the asset
* itself, but for bitassets some of the fees are denominated in the collateral
* asset. This will place the fee in the right container.
*/
template<class DB>
void accumulate_fee(DB& db, const asset& fee) const
{
abitmore marked this conversation as resolved.
Show resolved Hide resolved
if (fee.amount == 0) return;
FC_ASSERT( fee.amount >= 0, "Fee amount must be non-negative." );
const auto& dyn_data = dynamic_asset_data_id(db);
if (fee.asset_id == get_id()) { // fee same as asset
db.modify( dyn_data, [&fee]( asset_dynamic_data_object& obj ){
obj.accumulated_fees += fee.amount;
});
} else { // fee different asset; perhaps collateral-denominated fee
FC_ASSERT( is_market_issued(),
"Asset ${a} (${id}) cannot accept fee of asset (${fid}).",
("a",this->symbol)("id",this->id)("fid",fee.asset_id) );
const auto & bad = bitasset_data(db);
FC_ASSERT( fee.asset_id == bad.options.short_backing_asset,
"Asset ${a} (${id}) cannot accept fee of asset (${fid}).",
("a",this->symbol)("id",this->id)("fid",fee.asset_id) );
db.modify( dyn_data, [&fee]( asset_dynamic_data_object& obj ){
obj.accumulated_collateral_fees += fee.amount;
});
}
}

};

/**
Expand Down
5 changes: 5 additions & 0 deletions libraries/chain/proposal_evaluator.cpp
Expand Up @@ -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.

}

struct proposal_operation_hardfork_visitor
Expand Down Expand Up @@ -61,6 +62,10 @@ struct proposal_operation_hardfork_visitor
detail::check_asset_options_hf_bsip81(block_time, v.new_options);
}

void operator()(const graphene::chain::asset_claim_fees_operation &v) const {
detail::check_asset_claim_fees_hardfork_87_74_collatfee(block_time, v); // HF_REMOVABLE
}

void operator()(const graphene::chain::committee_member_update_global_parameters_operation &op) const {
if (block_time < HARDFORK_CORE_1468_TIME) {
FC_ASSERT(!op.new_parameters.extensions.value.updatable_htlc_options.valid(), "Unable to set HTLC options before hardfork 1468");
Expand Down
3 changes: 3 additions & 0 deletions libraries/protocol/asset_ops.cpp
Expand Up @@ -251,6 +251,8 @@ void asset_options::validate()const
void asset_claim_fees_operation::validate()const {
FC_ASSERT( fee.amount >= 0 );
FC_ASSERT( amount_to_claim.amount > 0 );
if( extensions.value.claim_from_asset_id.valid() )
FC_ASSERT( *extensions.value.claim_from_asset_id != amount_to_claim.asset_id );
}

void asset_claim_pool_operation::validate()const {
Expand All @@ -271,6 +273,7 @@ GRAPHENE_IMPLEMENT_EXTERNAL_SERIALIZATION( graphene::protocol::asset_settle_oper
GRAPHENE_IMPLEMENT_EXTERNAL_SERIALIZATION( graphene::protocol::asset_fund_fee_pool_operation::fee_parameters_type )
GRAPHENE_IMPLEMENT_EXTERNAL_SERIALIZATION( graphene::protocol::asset_claim_pool_operation::fee_parameters_type )
GRAPHENE_IMPLEMENT_EXTERNAL_SERIALIZATION( graphene::protocol::asset_claim_fees_operation::fee_parameters_type )
GRAPHENE_IMPLEMENT_EXTERNAL_SERIALIZATION( graphene::protocol::asset_claim_fees_operation::additional_options_type )
GRAPHENE_IMPLEMENT_EXTERNAL_SERIALIZATION( graphene::protocol::asset_update_operation::fee_parameters_type )
GRAPHENE_IMPLEMENT_EXTERNAL_SERIALIZATION( graphene::protocol::asset_update_issuer_operation::fee_parameters_type )
GRAPHENE_IMPLEMENT_EXTERNAL_SERIALIZATION( graphene::protocol::asset_update_bitasset_operation::fee_parameters_type )
Expand Down
20 changes: 17 additions & 3 deletions libraries/protocol/include/graphene/protocol/asset_ops.hpp
Expand Up @@ -446,10 +446,21 @@ namespace graphene { namespace protocol {
uint64_t fee = 20 * GRAPHENE_BLOCKCHAIN_PRECISION;
};

struct additional_options_type
{
/// Which asset to claim fees from. This is needed, e.g., to claim collateral-
/// denominated fees from a collateral-backed smart asset. If unset, assumed to be same
/// asset as amount_to_claim is denominated in, such as would be the case when claiming
/// market fees. If set, validation requires it to be a different asset_id than
/// amount_to_claim (else there would exist two ways to form the same request).
fc::optional<asset_id_type> claim_from_asset_id;
abitmore marked this conversation as resolved.
Show resolved Hide resolved
};

asset fee;
account_id_type issuer;
asset amount_to_claim; /// amount_to_claim.asset_id->issuer must == issuer
extensions_type extensions;
account_id_type issuer; ///< must match issuer of asset from which we claim fees
asset amount_to_claim;

extension<additional_options_type> extensions;

account_id_type fee_payer()const { return issuer; }
void validate()const;
Expand Down Expand Up @@ -521,6 +532,8 @@ namespace graphene { namespace protocol {

FC_REFLECT( graphene::protocol::asset_claim_fees_operation, (fee)(issuer)(amount_to_claim)(extensions) )
FC_REFLECT( graphene::protocol::asset_claim_fees_operation::fee_parameters_type, (fee) )
FC_REFLECT( graphene::protocol::asset_claim_fees_operation::additional_options_type, (claim_from_asset_id) )

FC_REFLECT( graphene::protocol::asset_claim_pool_operation, (fee)(issuer)(asset_id)(amount_to_claim)(extensions) )
FC_REFLECT( graphene::protocol::asset_claim_pool_operation::fee_parameters_type, (fee) )

Expand Down Expand Up @@ -619,6 +632,7 @@ GRAPHENE_DECLARE_EXTERNAL_SERIALIZATION( graphene::protocol::asset_settle_operat
GRAPHENE_DECLARE_EXTERNAL_SERIALIZATION( graphene::protocol::asset_fund_fee_pool_operation::fee_parameters_type )
GRAPHENE_DECLARE_EXTERNAL_SERIALIZATION( graphene::protocol::asset_claim_pool_operation::fee_parameters_type )
GRAPHENE_DECLARE_EXTERNAL_SERIALIZATION( graphene::protocol::asset_claim_fees_operation::fee_parameters_type )
GRAPHENE_DECLARE_EXTERNAL_SERIALIZATION( graphene::protocol::asset_claim_fees_operation::additional_options_type )
GRAPHENE_DECLARE_EXTERNAL_SERIALIZATION( graphene::protocol::asset_update_operation::fee_parameters_type )
GRAPHENE_DECLARE_EXTERNAL_SERIALIZATION( graphene::protocol::asset_update_issuer_operation::fee_parameters_type )
GRAPHENE_DECLARE_EXTERNAL_SERIALIZATION( graphene::protocol::asset_update_bitasset_operation::fee_parameters_type )
Expand Down
1 change: 1 addition & 0 deletions tests/common/database_fixture.cpp
Expand Up @@ -529,6 +529,7 @@ void database_fixture::verify_asset_supplies( const database& db )
{
const auto& bad = asset_obj.bitasset_data(db);
total_balances[bad.options.short_backing_asset] += bad.settlement_fund;
total_balances[bad.options.short_backing_asset] += dasset_obj.accumulated_collateral_fees;
}
total_balances[asset_obj.id] += dasset_obj.confidential_supply.value;
}
Expand Down