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

Asset claim pool operation #572

Merged
merged 22 commits into from Feb 23, 2018
Merged
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
8f24bca
Implemented the new operation asset_claim_pool_operation
Dec 18, 2017
6435e51
Add visitor to check pre-HF use of new operation
xeroc Jan 12, 2018
b09d338
Remove whitespaces
xeroc Jan 17, 2018
7e78949
Change hardfork name
xeroc Jan 18, 2018
a473a9e
Hard fork times updated in fee tests
xeroc Jan 18, 2018
e6506d2
fixes errors and added fee_helper for asset_claim_pool_operation
xeroc Jan 22, 2018
83ff729
Fixes tests, typos and wront operations
xeroc Jan 23, 2018
a9ef41c
fix typo in hardfork comment
xeroc Jan 24, 2018
80d2a2e
improved unittests and fixed hardfork conditions
xeroc Jan 26, 2018
9a424ce
fix assert message
xeroc Feb 9, 2018
dba7a53
fix terminology in documentation
xeroc Feb 9, 2018
c260938
Also test the hf_xxx_visitor thru a proposal
xeroc Feb 9, 2018
ba54247
Test claim pool with proposal after hard fork #188
abitmore Feb 13, 2018
119dc91
[tests] properly reload objects after new blocks have been generated
xeroc Feb 13, 2018
c767e0c
[tests] properly reload objects after new blocks have been generated …
xeroc Feb 13, 2018
563ed57
Add missing semicolon
xeroc Feb 14, 2018
8e41fab
[tests] Fix unsafe variables in unit test
xeroc Feb 14, 2018
53b2404
[test] Make sure to have a safe reference to core_asset after block g…
xeroc Feb 14, 2018
294f918
increase hardfork time during development
xeroc Feb 22, 2018
716638d
Do not capture all in lambda
xeroc Feb 22, 2018
a8c1584
Fix terminology of variables in cli_wallet
xeroc Feb 22, 2018
77dfdf8
Do not use reference on *_id_type
xeroc Feb 22, 2018
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -43,6 +43,7 @@ struct get_impacted_account_visitor
}

void operator()( const asset_claim_fees_operation& op ){}
void operator()( const asset_claim_pool_operation& op ){}
void operator()( const limit_order_create_operation& op ) {}
void operator()( const limit_order_cancel_operation& op )
{
@@ -616,4 +616,31 @@ void_result asset_claim_fees_evaluator::do_apply( const asset_claim_fees_operati
} FC_CAPTURE_AND_RETHROW( (o) ) }


void_result asset_claim_pool_evaluator::do_evaluate( const asset_claim_pool_operation& o )
{ try {
FC_ASSERT( db().head_block_time() >= HARDFORK_CORE_188_TIME,
"This operation is only available after Hardfork #188!" );
FC_ASSERT( o.asset_id(db()).issuer == o.issuer, "Asset fee pool may only be claimed by the issuer" );

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

void_result asset_claim_pool_evaluator::do_apply( const asset_claim_pool_operation& o )
{ try {
database& d = db();

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

d.modify( addo, [&]( asset_dynamic_data_object& _addo ) {

This comment has been minimized.

Copy link
@pmconrad

pmconrad Feb 21, 2018

Contributor

We should introduce a new rule: instead of the catch-all [&], be as restrictive as possible. Prefer capturing by value, and capture only what's required.

Reason: capturing by reference is inherently dangerous, because the reference may have gone out of scope by the time the lambda executes. (This doesn't happen in d.modify because modify returns only after the lambda has been executed. Nevertheless, avoiding the catch-all is a healthy practice and avoids complaints from static code analysis tools.)

_addo.fee_pool -= o.amount_to_claim.amount;
});

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

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


} } // graphene::chain
@@ -172,6 +172,7 @@ void database::initialize_evaluators()
register_evaluator<transfer_from_blind_evaluator>();
register_evaluator<blind_transfer_evaluator>();
register_evaluator<asset_claim_fees_evaluator>();
register_evaluator<asset_claim_pool_evaluator>();
}

void database::initialize_indexes()
@@ -31,6 +31,7 @@ struct get_impacted_account_visitor
}

void operator()( const asset_claim_fees_operation& op ){}
void operator()( const asset_claim_pool_operation& op ){}
void operator()( const limit_order_create_operation& op ) {}
void operator()( const limit_order_cancel_operation& op )
{
@@ -0,0 +1,4 @@
// #188 Add operation to allow claiming of funds in an asset's fee pool
#ifndef HARDFORK_CORE_188_TIME
#define HARDFORK_CORE_188_TIME (fc::time_point_sec( 1446652800 ))

This comment has been minimized.

Copy link
@pmconrad

pmconrad Feb 21, 2018

Contributor

Please change this to a date in the far future.

#endif
@@ -152,4 +152,32 @@ namespace graphene { namespace chain {
void_result do_apply( const asset_claim_fees_operation& o );
};

class asset_claim_pool_evaluator : public evaluator<asset_claim_pool_evaluator>
{
public:
typedef asset_claim_pool_operation operation_type;

void_result do_evaluate( const asset_claim_pool_operation& o );
void_result do_apply( const asset_claim_pool_operation& o );
};

namespace impl { // TODO: remove after HARDFORK_CORE_188_TIME has passed
class hf_188_visitor {
public:
typedef void result_type;

template<typename T>
void operator()( const T& v )const {}

void operator()( const graphene::chain::asset_claim_pool_operation& v )const {
FC_ASSERT( false, "Not allowed until hardfork 188" );
}

void operator()( const graphene::chain::proposal_create_operation& v )const {
for( const op_wrapper& op : v.proposed_ops )
op.op.visit( *this );
}
};
}

} } // graphene::chain
@@ -27,6 +27,8 @@
#include <graphene/chain/evaluator.hpp>
#include <graphene/chain/database.hpp>
#include <graphene/chain/transaction_evaluation_state.hpp>
#include <graphene/chain/asset_evaluator.hpp>
#include <graphene/chain/hardfork.hpp>

namespace graphene { namespace chain {

@@ -442,11 +442,42 @@ namespace graphene { namespace chain {
void validate()const;
};

/**
* @brief Transfers BTS from the fee pool of a specified asset back to the issuer's balance
* @param fee Payment for the operation execution
* @param issuer Account which will be used for transfering BTS
* @param asset_id Id of the asset whose fee pool is going to be drained
* @param amount_to_claim Amount of BTS to claim from the fee pool
* @param extensions Field for future expansion
* @pre @ref fee must be paid in the asset other than the one whose pool is being drained
* @pre @ref amount_to_claim should be specified in the core asset
* @pre @ref amount_to_claim should be nonnegative
*/
struct asset_claim_pool_operation : public base_operation
{
struct fee_parameters_type {
uint64_t fee = 20 * GRAPHENE_BLOCKCHAIN_PRECISION;

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 20, 2018

Member

The default fee is perhaps too high. Although it can be changed by the committee later, I think it's better be handled like bid_collateral_operation (code).

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 22, 2018

Author Member

This was interesting. I added the code as requested. I assume the code does the following: Figure out if the blockchain parameters have a fee for that operation already, or otherwise pick another fee for it instead. In my case, i chose the fee for asset_fund_fee_pool_operation

};

asset fee;
account_id_type issuer;
asset_id_type asset_id; /// fee.asset_id must != asset_id
asset amount_to_claim; /// core asset

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jan 12, 2018

Contributor

It would be sufficient to specify the amount only, since asset_id is fixed anyway.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 17, 2018

Author Member

Correct ... asset contains asset_id ..
Why not change asset to share_type then?

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 17, 2018

Author Member

How would the blockchain know which asset I want to drain the fee pool of?
We can discuss whether amount_to_claim should be share_type because it will always be CORE/1.3.0 ... but to identify the fee pool, i need the asset_id, no?

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jan 17, 2018

Contributor

I meant the amount_to_claim.asset_id - that's always the core asset id.

extensions_type extensions;

account_id_type fee_payer()const { return issuer; }
void validate()const;
};


} } // graphene::chain

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

FC_REFLECT( graphene::chain::asset_options,
(max_supply)
@@ -77,6 +77,21 @@ namespace graphene { namespace chain {
}
};

template<>
class fee_helper<asset_claim_pool_operation> {
public:
const asset_claim_pool_operation::fee_parameters_type& cget(const flat_set<fee_parameters>& parameters)const
{
auto itr = parameters.find( asset_claim_pool_operation::fee_parameters_type() );
if ( itr != parameters.end() )
return itr->get<asset_claim_pool_operation::fee_parameters_type>();

static asset_claim_pool_operation::fee_parameters_type asset_claim_pool_dummy;
asset_claim_pool_dummy.fee = fee_helper<asset_fund_fee_pool_operation>().cget(parameters).fee;
return asset_claim_pool_dummy;
}
};

/**
* @brief contains all of the parameters necessary to calculate the fee for any operation
*/
@@ -93,7 +93,8 @@ namespace graphene { namespace chain {
asset_claim_fees_operation,
fba_distribute_operation, // VIRTUAL
bid_collateral_operation,
execute_bid_operation // VIRTUAL
execute_bid_operation, // VIRTUAL
asset_claim_pool_operation
> operation;

/// @} // operations group
@@ -26,6 +26,8 @@
#include <graphene/chain/account_object.hpp>
#include <graphene/chain/protocol/fee_schedule.hpp>
#include <graphene/chain/exceptions.hpp>
#include <graphene/chain/asset_evaluator.hpp>


#include <fc/smart_ref_impl.hpp>

@@ -75,6 +77,12 @@ void_result proposal_create_evaluator::do_evaluate(const proposal_create_operati
_proposed_trx.operations.push_back(op.op);
_proposed_trx.validate();

if( d.head_block_time() < HARDFORK_CORE_188_TIME )
{ // TODO: remove after HARDFORK_CORE_188_TIME has passed
graphene::chain::impl::hf_188_visitor hf_188;
hf_188( o );
}

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

@@ -234,4 +234,11 @@ void asset_claim_fees_operation::validate()const {
FC_ASSERT( amount_to_claim.amount > 0 );
}

void asset_claim_pool_operation::validate()const {
FC_ASSERT( fee.amount >= 0 );
FC_ASSERT( fee.asset_id != asset_id);
FC_ASSERT( amount_to_claim.amount > 0 );
FC_ASSERT( amount_to_claim.asset_id == asset_id_type());
}

} } // namespace graphene::chain
@@ -1113,6 +1113,23 @@ class wallet_api
string amount,
bool broadcast = false);

/** Claim funds from the fee pool for the given asset.
*
* User-issued assets can optionally have a pool of the core asset which is
* automatically used to pay transaction fees for any transaction using that
* asset (using the asset's core exchange rate).
*
* This command allows the issuer to withdraw those funds from the fee pool.
*
* @param symbol the name or id of the asset whose fee pool you wish to claim
* @param amount the amount of the core asset to withdraw
* @param broadcast true to broadcast the transaction on the network
* @returns the signed transaction claiming from the fee pool
*/
signed_transaction claim_asset_fee_pool(string symbol,
string amount,
bool broadcast = false);

/** Burns the given user-issued asset.
*
* This command burns the user-issued asset to reduce the amount in circulation.
@@ -1663,6 +1680,7 @@ FC_API( graphene::wallet::wallet_api,
(get_asset)
(get_bitasset_data)
(fund_asset_fee_pool)
(claim_asset_fee_pool)
(reserve_asset)
(global_settle_asset)
(settle_asset)
@@ -1269,6 +1269,29 @@ class wallet_api_impl
return sign_transaction( tx, broadcast );
} FC_CAPTURE_AND_RETHROW( (from)(symbol)(amount)(broadcast) ) }

signed_transaction claim_asset_fee_pool(string symbol,
string amount,
bool broadcast /* = false */)
{ try {
optional<asset_object> asset_to_fund = find_asset(symbol);

This comment has been minimized.

Copy link
@pmconrad

pmconrad Feb 21, 2018

Contributor

asset_to_fund is not a good name, I suppose that's copy&pasted from fund_fee_pool...

if (!asset_to_fund)
FC_THROW("No asset with that symbol exists!");
asset_object core_asset = get_asset(asset_id_type());

asset_claim_pool_operation claim_op;
claim_op.issuer = asset_to_fund->issuer;
claim_op.asset_id = asset_to_fund->id;
claim_op.amount_to_claim = core_asset.amount_from_string(amount).amount;

signed_transaction tx;
tx.operations.push_back( claim_op );
set_operation_fees( tx, _remote_db->get_global_properties().parameters.current_fees);
tx.validate();

return sign_transaction( tx, broadcast );
} FC_CAPTURE_AND_RETHROW( (symbol)(amount)(broadcast) ) }


signed_transaction reserve_asset(string from,
string amount,
string symbol,
@@ -3332,6 +3355,13 @@ signed_transaction wallet_api::fund_asset_fee_pool(string from,
return my->fund_asset_fee_pool(from, symbol, amount, broadcast);
}

signed_transaction wallet_api::claim_asset_fee_pool(string symbol,
string amount,
bool broadcast /* = false */)
{
return my->claim_asset_fee_pool(symbol, amount, broadcast);
}

signed_transaction wallet_api::reserve_asset(string from,
string amount,
string symbol,
@@ -897,6 +897,7 @@ void database_fixture::fund_fee_pool( const account_object& from, const asset_ob

for( auto& op : trx.operations ) db.current_fee_schedule().set_fee(op);
trx.validate();
set_expiration( db, trx );
db.push_transaction(trx, ~0);
trx.operations.clear();
verify_asset_supplies(db);
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.