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 update issuer operation #599

Merged
merged 15 commits into from
Mar 7, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions libraries/app/impacted.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ struct get_impacted_account_visitor
_impacted.insert( *(op.new_issuer) );
}

void operator()( const asset_update_issuer_operation& op ) {
_impacted.insert( op.new_issuer );
}

void operator()( const asset_update_bitasset_operation& op ) {}
void operator()( const asset_update_feed_producers_operation& op ) {}

Expand Down
66 changes: 52 additions & 14 deletions libraries/chain/asset_evaluator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,23 @@ void_result asset_fund_fee_pool_evaluator::do_apply(const asset_fund_fee_pool_op
return void_result();
} FC_CAPTURE_AND_RETHROW( (o) ) }

static void validate_new_issuer( const database& d, const asset_object& a, account_id_type new_issuer )
Copy link
Member

Choose a reason for hiding this comment

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

What's the static for?

Copy link
Contributor

Choose a reason for hiding this comment

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

That avoids exposing the symbol outside the compilation unit.

{ try {
FC_ASSERT(d.find_object(new_issuer));
if( a.is_market_issued() && new_issuer == GRAPHENE_COMMITTEE_ACCOUNT )
{
const asset_object& backing = a.bitasset_data(d).options.short_backing_asset(d);
if( backing.is_market_issued() )
{
const asset_object& backing_backing = backing.bitasset_data(d).options.short_backing_asset(d);
FC_ASSERT( backing_backing.get_id() == asset_id_type(),
"May not create a blockchain-controlled market asset which is not backed by CORE.");
} else
FC_ASSERT( backing.get_id() == asset_id_type(),
"May not create a blockchain-controlled market asset which is not backed by CORE.");
}
} FC_CAPTURE_AND_RETHROW( (a)(new_issuer) ) }

void_result asset_update_evaluator::do_evaluate(const asset_update_operation& o)
{ try {
database& d = db();
Expand All @@ -262,19 +279,9 @@ void_result asset_update_evaluator::do_evaluate(const asset_update_operation& o)

if( o.new_issuer )
{
FC_ASSERT(d.find_object(*o.new_issuer));
if( a.is_market_issued() && *o.new_issuer == GRAPHENE_COMMITTEE_ACCOUNT )
{
const asset_object& backing = a.bitasset_data(d).options.short_backing_asset(d);
if( backing.is_market_issued() )
{
const asset_object& backing_backing = backing.bitasset_data(d).options.short_backing_asset(d);
FC_ASSERT( backing_backing.get_id() == asset_id_type(),
"May not create a blockchain-controlled market asset which is not backed by CORE.");
} else
FC_ASSERT( backing.get_id() == asset_id_type(),
"May not create a blockchain-controlled market asset which is not backed by CORE.");
}
FC_ASSERT( d.head_block_time() < HARDFORK_CORE_199_TIME,
Copy link
Contributor

Choose a reason for hiding this comment

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

Be consistent with the HF check, elsewhere you use <=

Copy link
Member Author

Choose a reason for hiding this comment

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

So true .. I made it < consistently so that I don't need to generate an 'extra block' in the unittests

"Since Hardfork #199, updating issuer requires the use of asset_update_issuer_operation.");
validate_new_issuer( d, a, *o.new_issuer );
}

if( (d.head_block_time() < HARDFORK_572_TIME) || (a.dynamic_asset_data_id(d).current_supply != 0) )
Expand All @@ -289,7 +296,7 @@ void_result asset_update_evaluator::do_evaluate(const asset_update_operation& o)
"Flag change is forbidden by issuer permissions");

asset_to_update = &a;
FC_ASSERT( o.issuer == a.issuer, "", ("o.issuer", o.issuer)("a.issuer", a.issuer) );
FC_ASSERT( o.issuer == a.issuer, "Incorrect issuer for asset! (${o.issuer} != ${a.issuer})", ("o.issuer", o.issuer)("a.issuer", a.issuer) );
Copy link
Member

Choose a reason for hiding this comment

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

This line is too long, better add a line break.


const auto& chain_parameters = d.get_global_properties().parameters;

Expand Down Expand Up @@ -328,6 +335,37 @@ void_result asset_update_evaluator::do_apply(const asset_update_operation& o)
return void_result();
} FC_CAPTURE_AND_RETHROW( (o) ) }

void_result asset_update_issuer_evaluator::do_evaluate(const asset_update_issuer_operation& o)
{ try {
database& d = db();

const asset_object& a = o.asset_to_update(d);

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the two lines with a_copy, they are useless here.

validate_new_issuer( d, a, o.new_issuer );

asset_to_update = &a;
FC_ASSERT( o.issuer == a.issuer, "Incorrect issuer for asset! (${o.issuer} != ${a.issuer})", ("o.issuer", o.issuer)("a.issuer", a.issuer) );
Copy link
Member

Choose a reason for hiding this comment

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

This line is too long, better add a line break.


if( d.head_block_time() < HARDFORK_CORE_199_TIME )
{ // TODO: remove after HARDFORK_CORE_199_TIME has passed
graphene::chain::impl::hf_199_visitor hf_199;
hf_199( o );
}


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

void_result asset_update_issuer_evaluator::do_apply(const asset_update_issuer_operation& o)
{ try {
database& d = db();
d.modify(*asset_to_update, [&](asset_object& a) {
a.issuer = o.new_issuer;
});

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

void_result asset_update_bitasset_evaluator::do_evaluate(const asset_update_bitasset_operation& o)
{ try {
database& d = db();
Expand Down
1 change: 1 addition & 0 deletions libraries/chain/db_init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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_update_issuer_evaluator>();
register_evaluator<asset_claim_pool_evaluator>();
}

Expand Down
5 changes: 4 additions & 1 deletion libraries/chain/db_notify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,10 @@ struct get_impacted_account_visitor
if( op.new_issuer )
_impacted.insert( *(op.new_issuer) );
}

void operator()( const asset_update_issuer_operation& op )
{
_impacted.insert( op.new_issuer );
}
void operator()( const asset_update_bitasset_operation& op ) {}
void operator()( const asset_update_feed_producers_operation& op ) {}

Expand Down
4 changes: 4 additions & 0 deletions libraries/chain/hardfork.d/CORE_199.hf
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// bitshares-core #199 Require owner key for change of asset-issuer (new operation)
#ifndef HARDFORK_CORE_199_TIME
#define HARDFORK_CORE_199_TIME (fc::time_point_sec( 1600000000 ))
#endif
36 changes: 27 additions & 9 deletions libraries/chain/include/graphene/chain/asset_evaluator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,17 @@ namespace graphene { namespace chain {
const asset_object* asset_to_update = nullptr;
};

class asset_update_issuer_evaluator : public evaluator<asset_update_issuer_evaluator>
{
public:
typedef asset_update_issuer_operation operation_type;

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

const asset_object* asset_to_update = nullptr;
};

class asset_update_bitasset_evaluator : public evaluator<asset_update_bitasset_evaluator>
{
public:
Expand Down Expand Up @@ -152,16 +163,24 @@ 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>
Copy link
Member

Choose a reason for hiding this comment

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

Should not remove this class.

{
public:
typedef asset_claim_pool_operation operation_type;
namespace impl { // TODO: remove after HARDFORK_CORE_199_TIME has passed
class hf_199_visitor {
public:
typedef void result_type;

void_result do_evaluate( const asset_claim_pool_operation& o );
void_result do_apply( const asset_claim_pool_operation& o );
};
template<typename T>
void operator()( const T& v )const {}

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

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

namespace impl { // TODO: remove after HARDFORK_CORE_188_TIME has passed
class hf_188_visitor {
public:
typedef void result_type;
Expand All @@ -179,5 +198,4 @@ namespace graphene { namespace chain {
}
};
}

} } // graphene::chain
43 changes: 42 additions & 1 deletion libraries/chain/include/graphene/chain/protocol/asset_ops.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,40 @@ namespace graphene { namespace chain {
void validate()const;
};

/**
Copy link
Member

Choose a reason for hiding this comment

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

This line is redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shit, it seems I must be much much more careful when merging to resolve conflicts. On it ..

/**
* @brief Update issuer of an asset
* @ingroup operations
*
* An issuer has general administrative power of an asset and in some cases
* also its shares issued to individuals. Thus, changing the issuer today
* requires the use of a separate operation that needs to be signed by the
* owner authority.
*
*/
struct asset_update_issuer_operation : public base_operation
{
struct fee_parameters_type {
uint64_t fee = 20 * GRAPHENE_BLOCKCHAIN_PRECISION;
};

asset fee;
account_id_type issuer;
asset_id_type asset_to_update;
account_id_type new_issuer;
extensions_type extensions;

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

void get_required_owner_authorities( flat_set<account_id_type>& a )const
{ a.insert( issuer ); }

void get_required_active_authorities( flat_set<account_id_type>& a )const
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can (IMHO) remove the get_required_active_authorities part, but the get_required_owner_authorities is crucial as we only want owner key to be able to sign that transction.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, not a big deal if keep it.

{ }

};

/**
* @brief Transfers BTS from the fee pool of a specified asset back to the issuer's balance

Expand Down Expand Up @@ -471,7 +505,6 @@ namespace graphene { namespace chain {
void validate()const;
};


} } // graphene::chain

FC_REFLECT( graphene::chain::asset_claim_fees_operation, (fee)(issuer)(amount_to_claim)(extensions) )
Expand Down Expand Up @@ -510,6 +543,7 @@ FC_REFLECT( graphene::chain::asset_settle_operation::fee_parameters_type, (fee)
FC_REFLECT( graphene::chain::asset_settle_cancel_operation::fee_parameters_type, )
FC_REFLECT( graphene::chain::asset_fund_fee_pool_operation::fee_parameters_type, (fee) )
FC_REFLECT( graphene::chain::asset_update_operation::fee_parameters_type, (fee)(price_per_kbyte) )
FC_REFLECT( graphene::chain::asset_update_issuer_operation::fee_parameters_type, (fee) )
FC_REFLECT( graphene::chain::asset_update_bitasset_operation::fee_parameters_type, (fee) )
FC_REFLECT( graphene::chain::asset_update_feed_producers_operation::fee_parameters_type, (fee) )
FC_REFLECT( graphene::chain::asset_publish_feed_operation::fee_parameters_type, (fee) )
Expand All @@ -535,6 +569,13 @@ FC_REFLECT( graphene::chain::asset_update_operation,
(new_options)
(extensions)
)
FC_REFLECT( graphene::chain::asset_update_issuer_operation,
(fee)
(issuer)
(asset_to_update)
(new_issuer)
(extensions)
)
FC_REFLECT( graphene::chain::asset_update_bitasset_operation,
(fee)
(issuer)
Expand Down
14 changes: 14 additions & 0 deletions libraries/chain/include/graphene/chain/protocol/fee_schedule.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,20 @@ namespace graphene { namespace chain {
};

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

static asset_update_issuer_operation::fee_parameters_type dummy;
dummy.fee = fee_helper<asset_update_operation>().cget(parameters).fee;
return dummy;
}
};

class fee_helper<asset_claim_pool_operation> {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe lack of a template<> above this line?

public:
const asset_claim_pool_operation::fee_parameters_type& cget(const flat_set<fee_parameters>& parameters)const
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ namespace graphene { namespace chain {
fba_distribute_operation, // VIRTUAL
bid_collateral_operation,
execute_bid_operation, // VIRTUAL
asset_update_issuer_operation,
asset_claim_pool_operation
> operation;

Expand Down
6 changes: 6 additions & 0 deletions libraries/chain/proposal_evaluator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <graphene/chain/protocol/fee_schedule.hpp>
#include <graphene/chain/exceptions.hpp>
#include <graphene/chain/asset_evaluator.hpp>
#include <graphene/chain/hardfork.hpp>


#include <fc/smart_ref_impl.hpp>
Expand Down Expand Up @@ -77,6 +78,11 @@ 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_199_TIME )
{ // TODO: remove after HARDFORK_CORE_199_TIME has passed
graphene::chain::impl::hf_199_visitor hf_199;
hf_199( o );
Copy link
Member

Choose a reason for hiding this comment

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

I think this won't compile.. Lack of a }.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for that


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;
Expand Down
6 changes: 6 additions & 0 deletions libraries/chain/protocol/asset_ops.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,12 @@ void asset_update_operation::validate()const
FC_ASSERT(dummy.asset_id == asset_id_type());
}

void asset_update_issuer_operation::validate()const
{
FC_ASSERT( fee.amount >= 0 );
FC_ASSERT(issuer != new_issuer);
Copy link
Member

Choose a reason for hiding this comment

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

These two lines have different code styles, looks strange.

}

share_type asset_update_operation::calculate_fee(const asset_update_operation::fee_parameters_type& k)const
{
return k.fee + calculate_data_fee( fc::raw::pack_size(*this), k.price_per_kbyte );
Expand Down
16 changes: 16 additions & 0 deletions libraries/wallet/include/graphene/wallet/wallet.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1043,6 +1043,21 @@ class wallet_api
asset_options new_options,
bool broadcast = false);

/** Update the issuer of an asset
* Since this call requires the owner authority of the current issuer to sign the transaction,
* a separated operation is used to change the issuer. This call simplifies the use of this action.
*
* @note This operation requires the owner key to be available in the wallet.
*
* @param symbol the name or id of the asset to update
* @param new_issuer if changing the asset's issuer, the name or id of the new issuer.
* @param broadcast true to broadcast the transaction on the network
* @returns the signed transaction updating the asset
*/
signed_transaction update_asset_issuer(string symbol,
string new_issuer,
bool broadcast = false);

/** Update the options specific to a BitAsset.
*
* BitAssets have some options which are not relevant to other asset types. This operation is used to update those
Expand Down Expand Up @@ -1679,6 +1694,7 @@ FC_API( graphene::wallet::wallet_api,
(get_transaction_id)
(create_asset)
(update_asset)
(update_asset_issuer)
(update_bitasset)
(update_asset_feed_producers)
(publish_asset_feed)
Expand Down
Loading