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

Conversation

@xeroc
Copy link
Member

commented Jan 11, 2018

Blockchain Projects BV would like to contrinute his first proposal to the backend development with an implementation of "asset_claim_pool_operation".

In contrast to BSIP27, which proposes to modify the existing operation to fund the asset fee pool to allow negative amounts for a refund, we decided that the better approach would be to add a new operation, not just because of a clear name of the operation, but also because we didn't want to touch existing code and change existing behavior.

Please feel free to give any kind of feedback.

BTW, this commits is considered a contribution to BitShares and is licensed under MIT, of course.

libraries/plugins/market_history/market_history_plugin.cpp Outdated
@@ -408,7 +408,7 @@ void market_history_plugin::plugin_set_program_options(
)
{
cli.add_options()
("bucket-size", boost::program_options::value<string>()->default_value("[60,300,900,1800,3600,14400,86400]"),
("bucket-size", boost::program_options::value<string>()->default_value("[15,60,300,3600,86400]"),

This comment has been minimized.

Copy link
@oxarbitrage

oxarbitrage Jan 11, 2018

Member

i saw a comment from @HarukaMa about this but it seems it was deleted. he was pointing at this line as unintended and he is right, this should remain [60,300,900,1800,3600,14400,86400]

This comment has been minimized.

Copy link
@HarukaMa

HarukaMa Jan 11, 2018

Contributor

I was confused by f213db3 and thought this was intended though...

This comment has been minimized.

Copy link
@oxarbitrage

oxarbitrage Jan 11, 2018

Member

hmm, i am confused now as well, the commit you mention was a reverse of a mistake i made pushing code directly to master but if we merge this as it is it will reverse again, i am sure that is unintended. the change should be removed making sure [60,300,900,1800,3600,14400,86400] are the final settings.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 12, 2018

Author Member

This seems to be a github issue, i rebased to develop and force push. Don't see this changeset locally in git.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jan 12, 2018

Contributor

I can see it locally after pulling from origin.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 17, 2018

Author Member

fixed

@xeroc xeroc force-pushed the asset_claim_pool_operation branch to 9b27752 Jan 12, 2018

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.

@@ -947,7 +1028,7 @@ BOOST_AUTO_TEST_CASE( stealth_fba_test )
BOOST_AUTO_TEST_CASE( defaults_test )
{ try {
fee_schedule schedule;
const limit_order_create_operation::fee_parameters_type default_order_fee;
const limit_order_create_operation::fee_parameters_type default_order_fee {};

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jan 12, 2018

Contributor

Why did you change this?

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 17, 2018

Author Member

We have added the braces because otherwise the tests project was not compiled with Clang on MacOs. The problem was in the fact, that If a program calls for the default initialization of a const object, the type of an object should have a user-provided default constructor. Since fee_parameters_type structure doesn't provide a default constructor, we have added an explicit initializer to compile the tests binary. So for now, we have kept this change.

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 17, 2018

Member

I wonder why there isn't an implicit default constructor since all variables in the struct have been assigned a default value.

@abitmore
Copy link
Member

left a comment

  • Change HARDFORK_413 to HARDFORK_CORE_188
  • Need unit test code that trigger and catch the exception before hard fork time.
  • Need unit test code for the changes about proposal_create_operation.

According to https://github.com/bitshares/bitshares-core/wiki/Git-Flow,

  • since this is a consensus related change, should submit the PR against hardfork branch but not the develop branch
  • should rebase and remove the "mistake" commit.
@abitmore

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

Note: after #573 is merged, need to add back some removed code, otherwise it won't compile.

@abitmore abitmore added this to the 201803 - Consensus Changing Release milestone Jan 13, 2018

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2018

I'll rebase, fix and move the PR over to hardfork

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2018

Why it's HARDFORK_413?

What other tag works? Do we have an actual naming scheme here and a doc that I can read that up?

@xeroc xeroc force-pushed the asset_claim_pool_operation branch from 400a608 to 24bfc0d Jan 17, 2018

@xeroc xeroc changed the base branch from develop to hardfork Jan 17, 2018

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2018

The numbers in the HARDFORK_x constants usually refer to a github issue number. Since you don't have a github issue, you could also use HARDFORK_BSIP_27 for example. But 413 is meaningless.

@abitmore

This comment has been minimized.

Copy link
Member

commented Jan 17, 2018

There is an issue for this PR: #188, so could use HARDFORK_CORE_188_TIME.
I guess 413 refers to cryptonomex/graphene#413 which is not related to this PR.

@abitmore

This comment has been minimized.

Copy link
Member

commented Jan 17, 2018

By the way, @xeroc you can link the email address used in git to your github account.

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2018

There is an issue for this PR: #188, so could use HARDFORK_CORE_188_TIME.

Thanks. Did so

By the way, @xeroc you can link the email address used in git to your github account.

Thanks, did so too.

Updated the PR

@@ -174,4 +183,24 @@ namespace graphene { namespace chain {
}
};
}

namespace impl { // TODO: remove after HARDFORK_CORE_188_TIME has passed
class hf_413_visitor {

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 19, 2018

Member

Please rename all 413 related code to 188 as well.

// #413 Add operation to claim asset fees
#ifndef HARDFORK_413_TIME
#define HARDFORK_413_TIME (fc::time_point_sec( 1446652800 ))
#endif

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 19, 2018

Member

I think this commit is wrong. HARDFORK_413_TIME should not be removed, since it's being used somewhere else in the project.

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 20, 2018

Member

If we decide to remove the check about HARDFORK_413_TIME, we can remove this file as well.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 22, 2018

Author Member

Dang, my fault .. I thought my dev has added 413 for me .. didn't realize he reused an existing varaible

@@ -598,7 +598,7 @@ void_result asset_publish_feeds_evaluator::do_apply(const asset_publish_feed_ope

void_result asset_claim_fees_evaluator::do_evaluate( const asset_claim_fees_operation& o )
{ try {
FC_ASSERT( db().head_block_time() > HARDFORK_413_TIME );
FC_ASSERT( db().head_block_time() > HARDFORK_CORE_188_TIME );

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 19, 2018

Member

This change is wrong.

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 20, 2018

Member

Actually this line can be removed, since the hard fork time has passed.
But it's wrong to change it to 188.
If we decide to remove this line, we also need to update related unit test code.

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

@@ -598,7 +598,7 @@ void_result asset_publish_feeds_evaluator::do_apply(const asset_publish_feed_ope

void_result asset_claim_fees_evaluator::do_evaluate( const asset_claim_fees_operation& o )
{ try {
FC_ASSERT( db().head_block_time() > HARDFORK_413_TIME );
FC_ASSERT( db().head_block_time() > HARDFORK_CORE_188_TIME );

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 20, 2018

Member

Actually this line can be removed, since the hard fork time has passed.
But it's wrong to change it to 188.
If we decide to remove this line, we also need to update related unit test code.

// #413 Add operation to claim asset fees
#ifndef HARDFORK_413_TIME
#define HARDFORK_413_TIME (fc::time_point_sec( 1446652800 ))
#endif

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 20, 2018

Member

If we decide to remove the check about HARDFORK_413_TIME, we can remove this file as well.

@@ -174,12 +174,12 @@ BOOST_AUTO_TEST_CASE(asset_claim_fees_test)

}

if( db.head_block_time() <= HARDFORK_413_TIME )
if( db.head_block_time() <= HARDFORK_CORE_188_TIME )

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 20, 2018

Member

The change here and a few lines below are wrong.
If we decide to remove the check about HARDFORK_413_TIME, the unit test code here should be updated accordingly.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 22, 2018

Author Member

Fixed

BOOST_CHECK( alicecoin.dynamic_asset_data_id(db).fee_pool == _core(300).amount );

const asset_object& core_asset = asset_id_type()(db);

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 20, 2018

Member

Need unit test code that trigger and catch the exception before hard fork time.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 22, 2018

Author Member

I dublicated hardfork related code from another test and modified it accordingly. Does this qualify as a proper test?

@xeroc xeroc force-pushed the asset_claim_pool_operation branch to ea912be Jan 22, 2018

@abitmore
Copy link
Member

left a comment

Need more changes.

@@ -185,15 +185,15 @@ namespace graphene { namespace chain {
}

namespace impl { // TODO: remove after HARDFORK_CORE_188_TIME has passed
class hf_413_visitor {
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_fees_operation& v )const {

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 22, 2018

Member

Wrong operation.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 23, 2018

Author Member

Dang - thanks for that!

if( db.head_block_time() <= HARDFORK_CORE_188_TIME )
{
// can't claim pool before hardfork
GRAPHENE_REQUIRE_THROW( claim_pool( alice_id, bobcoin_id, _core(200), core_asset), fc::exception );

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 22, 2018

Member

Please modify the unit test case to test exactly what we want to test.
The asset was created before enabling fee, so the pool should be empty here, so an exception should be thrown out even if it's after hard fork time.
Also another exception would be thrown out since Alice can not claim bob's asset's fee pool.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 23, 2018

Author Member

There is a test for claiming a pool that doesn't belong to you in line 285

I've fixed the test that raises an exception before the hard fork to claim.
Now

  • trying to claim before the hardfork - which should fail due to hard fork time
  • trying to claim after the hardfork, but before funding which should fail due to now funds
  • trying to claim after the hardfork and after funding, but from non-issuer account which fails due to wrong account

Is that about right?

@@ -85,7 +85,7 @@ void_result proposal_create_evaluator::do_evaluate(const proposal_create_operati

if( d.head_block_time() <= HARDFORK_CORE_188_TIME )
{ // TODO: remove after HARDFORK_CORE_188_TIME has passed
graphene::chain::impl::hf_413_visitor hf_413;
graphene::chain::impl::hf_188_visitor hf_413;

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 22, 2018

Member

Better rename the variable as well.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 23, 2018

Author Member

Done

@@ -0,0 +1,4 @@
// #413 Add operation to claim asset fees

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 22, 2018

Member

Please revise this comment

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 23, 2018

Author Member

Done

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2018

Thanks for being so patient with me!

@abitmore
Copy link
Member

left a comment

Please fix new issues.

@@ -263,7 +263,7 @@ BOOST_AUTO_TEST_CASE(asset_claim_pool_test)
if( db.head_block_time() <= HARDFORK_CORE_188_TIME )
{
// can't claim pool before hardfork
GRAPHENE_REQUIRE_THROW( claim_pool( alice_id, bobcoin_id, _core(200), core_asset), fc::exception );
GRAPHENE_REQUIRE_THROW( claim_pool( alice_id, aliceusd.id, _core(200), core_asset), fc::exception );

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 23, 2018

Member

Perhaps I didn't make it clear enough. Should fund the fee pool with enough CORE before this test.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 24, 2018

Author Member

Well, that test fails because we are befor the hardfork.
After going forward to the hardfork right below that code, the next test fails because
the fee pool is not funded but we are after the hard fork.
Right after that, the fee pool is funded and we do further tests.

Am I misunderstanding something?

@@ -272,6 +272,8 @@ BOOST_AUTO_TEST_CASE(asset_claim_pool_test)
}
}

// can't claim pool because it is empty
GRAPHENE_REQUIRE_THROW( claim_pool( alice_id, aliceusd.id, _core(1), core_asset), fc::exception );

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 23, 2018

Member

I think this is unneeded, because there is already another test case below: "can't claim more than is available in the fee pool".

Although it doesn't harm to have it here right now, after fixed the issue above, the pool will no long be empty here so this test will fail.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 24, 2018

Author Member

Please read comment above

@@ -1,4 +1,4 @@
// #413 Add operation to claim asset fees
// #188 Add operation to allow claiming of funds in an asset's the fee pool

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 23, 2018

Member

Better remove "the"?

@abitmore

This comment has been minimized.

Copy link
Member

commented Jan 23, 2018

@xeroc can you please revise https://github.com/bitshares/bsips/blob/master/bsip-0027.md to match this implementation?

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2018

@xeroc can you please revise https://github.com/bitshares/bsips/blob/master/bsip-0027.md to match this implementation?

Of course, I can! On it ...

@abitmore
Copy link
Member

left a comment

Need to fix test case.

if( db.head_block_time() <= HARDFORK_CORE_188_TIME )
{
// can't claim pool before hardfork
GRAPHENE_REQUIRE_THROW( claim_pool( alice_id, aliceusd.id, _core(200), core_asset), fc::exception );

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 25, 2018

Member

You said:

that test fails because we are befor the hardfork.

No. If an exception is caught here, it's also possible that it's due to insufficient fund in the pool. Say, if you remove this line (to create an obvious bug), the test will pass too, which means the unit test is not well designed. To test the hard fork time logic, you should design a case that no exception will be thrown out due to other reason.

Another thing, as @pmconrad mentioned in another PR, test case should be deterministic, so if( db.head_block_time() <= HARDFORK_CORE_188_TIME ) doesn't make sense here.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 26, 2018

Author Member

Understood ... and fixed (hopefully)

share_type current_balance = get_balance( alice_id, asset_id_type() );
BOOST_CHECK( balance_before_claim + _core(100).amount == current_balance );

}

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 25, 2018

Member

Best if have a unit test case for proposal_create_operation.

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 26, 2018

Author Member

Erm, not sure how to actually do that .. You are basically asking for a test case for the hf_*_vistitor?

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 27, 2018

Member

Yes, something like the indirect test here in comparison to the direct test.

This comment has been minimized.

Copy link
@xeroc

xeroc Feb 9, 2018

Author Member

Would this be sufficient to test the visitor?

This comment has been minimized.

Copy link
@abitmore

abitmore Feb 9, 2018

Member

Still need another one test after hard fork. Thanks.

@@ -624,7 +624,8 @@ void_result asset_claim_fees_evaluator::do_apply( const asset_claim_fees_operati

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 );
FC_ASSERT( db().head_block_time() >= HARDFORK_CORE_188_TIME,
"This operation is only available hafter Hardfork #188!" );

This comment has been minimized.

Copy link
@abitmore

This comment has been minimized.

Copy link
@xeroc

xeroc Jan 30, 2018

Author Member

fixed

@abitmore

This comment has been minimized.

Copy link
Member

commented Jan 27, 2018

Due to #573, we need to re-add the including of header files to proposal_evaluator.hpp, otherwise it won't compile.

#include <graphene/chain/asset_evaluator.hpp>
#include <graphene/chain/hardfork.hpp>
@abitmore

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

Best if have a CLI command for this new op.

@abitmore
Copy link
Member

left a comment

        // can't claim pool because it is empty
        GRAPHENE_REQUIRE_THROW( claim_pool( alice_id, alicecoin.id, _core(1), core_asset), fc::exception );

Test failed.

@abitmore

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

@xeroc after generated a block, the pointers and references saved above will become invalid, you need to change all the pointers and references (after generate_block) to xxx_id(db).

fund_fee_pool( alice, alicecoin, _core(300).amount );

// Test amount of CORE in fee pools
BOOST_CHECK( alicecoin.dynamic_asset_data_id(db).fee_pool == _core(300).amount );

This comment has been minimized.

Copy link
@abitmore

abitmore Feb 13, 2018

Member

Better change this check (and checks for equality below) to
BOOST_CHECK_EQUAL( alicecoin.dynamic_asset_data_id(db).fee_pool.value, 300)

This comment has been minimized.

Copy link
@xeroc

xeroc Feb 13, 2018

Author Member

interestingly, the tests fail this way, but not when using _core().amount .. do I maybe need to cast to uint64, or so?

@@ -299,11 +299,11 @@ BOOST_AUTO_TEST_CASE(asset_claim_pool_test)
GRAPHENE_REQUIRE_THROW( claim_pool( alice_id, alicecoin.id, _core(1), core_asset), fc::exception );

// deposit 300 BTS to the fee pool of ALICECOIN asset
fund_fee_pool( alice, alicecoin, _core(300).amount );
fund_fee_pool( alice_id(db), alicecoin, _core(300).amount );

This comment has been minimized.

Copy link
@abitmore

abitmore Feb 13, 2018

Member

Need to change references to alicecoin and aliceusd as well.


// Test amount of CORE in fee pools
BOOST_CHECK( alicecoin.dynamic_asset_data_id(db).fee_pool == _core(300).amount );
BOOST_CHECK( aliceusd.dynamic_asset_data_id(db).fee_pool == _core(100).amount );
BOOST_CHECK( alicecoin.dynamic_asset_data_id(db).fee_pool.value == _core(300).amount );

This comment has been minimized.

Copy link
@abitmore

abitmore Feb 13, 2018

Member

No. I mean using BOOST_CHECK_EQUAL is better than BOOST_CHECK( a == b ) when possible. Actually my previous comment is not 100% correct because I overlooked core_prec.

This comment has been minimized.

Copy link
@abitmore

abitmore Feb 13, 2018

Member

BOOST_CHECK_EQUAL( alicecoin.dynamic_asset_data_id(db).fee_pool.value, 300*core_prec) would work.

This comment has been minimized.

Copy link
@abitmore

abitmore Feb 13, 2018

Member

This "would work" change looks ugly. I don't know why it times precision in the first place which makes the tests a bit complicated. Not a big deal anyway, so I think it's fine to revert this change. Thanks.

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2018

Changes made and tests are looking good:

make && ./chain_test --run_test=fee_tests/asset_claim_pool_test

Random number generator seeded to 1518541189
GRAPHENE_TESTING_GENESIS_TIMESTAMP is 1431700000
Running 1 test case...
3589915ms th_a       db_management.cpp:151         open                 ] Wiping object_database due to missing or wrong version
3589916ms th_a       object_database.cpp:93        wipe                 ] Wiping object database...
3589916ms th_a       object_database.cpp:95        wipe                 ] Done wiping object databse.
3589916ms th_a       object_database.cpp:106       open                 ] Opening object database from /tmp/graphene-tmp/a02e-8845-7907-1afd ...
3589916ms th_a       object_database.cpp:111       open                 ] Done opening object database.
3593844ms th_a       db_management.cpp:191         close                ] Rewinding from 3 to 0
3593844ms th_a       db_management.cpp:201         close                ] Database close unexpected exception: {"code":10,"name":"assert_exception","message":"Assert Exception","stack":[{"context":{"level":"error","file":"fork_database.cpp","line":41,"method":"pop_block","hostname":"","thread_name":"th_a","timestamp":"2018-02-13T16:59:53"},"format":"_head: no blocks to pop","data":{}},{"context":{"level":"warn","file":"db_block.cpp","line":430,"method":"pop_block","hostname":"","thread_name":"th_a","timestamp":"2018-02-13T16:59:53"},"format":"","data":{}}]}

*** No errors detected

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2018

Hmm .. too soon .. for some reason, the last test that claims via proposal results in a segfault:

unknown location(0): fatal error: in "fee_tests/asset_claim_pool_test": signal: SIGSEGV, si_code: 0 (memory access violation at address: 0x00000080)
/home/xeroc/BitShares/bitshares-2/tests/tests/fee_tests.cpp(331): last checkpoint

The code that I incommented that resulted in the segfault is this:

claim_pool_proposal( alice_id, aliceusd_id, _core(1), core_asset);

Why would it segfault?

// can claim pool with a proposal after hard fork
claim_pool_proposal( alice_id, aliceusd.id, _core(1), core_asset);
// can create a proposal to claim claim pool after hard fork
claim_pool_proposal( alice_id, aliceusd_id, _core(1), core_asset)

This comment has been minimized.

Copy link
@abitmore

abitmore Feb 13, 2018

Member

error: expected ‘;’ before ‘}’ token

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2018

I am still getting a

Random number generator seeded to 1518603344
GRAPHENE_TESTING_GENESIS_TIMESTAMP is 1431700000
Running 1 test case...
944830ms th_a       db_management.cpp:151         open                 ] Wiping object_database due to missing or wrong version
944830ms th_a       object_database.cpp:93        wipe                 ] Wiping object database...
944830ms th_a       object_database.cpp:95        wipe                 ] Done wiping object databse.
944830ms th_a       object_database.cpp:106       open                 ] Opening object database from /tmp/graphene-tmp/5c2e-36b7-a67d-a9e8 ...
944830ms th_a       object_database.cpp:111       open                 ] Done opening object database.
unknown location(0): fatal error: in "fee_tests/asset_claim_pool_test": signal: SIGSEGV, si_code: 0 (memory access violation at address: 0x00000080)
/home/xeroc/BitShares/bitshares-2/tests/tests/fee_tests.cpp(331): last checkpoint

*** 1 failure is detected in the test module "Master Test Suite"
enable_fees();

auto claim_pool = [&]( const account_id_type& issuer, const asset_id_type& asset_to_claim,
const asset& amount_to_fund, const asset_object& fee_asset )

This comment has been minimized.

Copy link
@abitmore

abitmore Feb 14, 2018

Member

Only big objects need to use references (&). Small objects like asset or asset_id_type don't need it.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Feb 21, 2018

Contributor

Makes sense for asset (because that's an object), but not for ..._id_type (as that's basically an int64).

claim_op.asset_id = asset_to_claim;
claim_op.amount_to_claim = amount_to_fund;

const auto proposal_create_fees = fees.get<proposal_create_operation>();

This comment has been minimized.

Copy link
@abitmore

abitmore Feb 14, 2018

Member

The reference to fees here is probably unsafe.
In addition, better use const auto& proposal_create_fees since it is a (somehow) big object.

GRAPHENE_REQUIRE_THROW( claim_pool( alice_id, alicecoin_id, _core(1), core_asset), fc::exception );

// deposit 300 BTS to the fee pool of ALICECOIN asset
fund_fee_pool( alice, alicecoin_id(db), _core(300).amount );

This comment has been minimized.

Copy link
@abitmore

abitmore Feb 14, 2018

Member

The reference to alice here is unsafe.

generate_blocks( HARDFORK_CORE_188_TIME );

// can't claim pool because it is empty
GRAPHENE_REQUIRE_THROW( claim_pool( alice_id, alicecoin_id, _core(1), core_asset), fc::exception );

This comment has been minimized.

Copy link
@abitmore

abitmore Feb 14, 2018

Member

The reference to core_asset here and below is probably unsafe, although I'm 99% sure it's safe.

@oxarbitrage
Copy link
Member

left a comment

this seems to be working perfect @xeroc, a great job.

@abitmore

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

I'm fine with this PR so far, best if @pmconrad can take a look.

@@ -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.

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.)

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...

enable_fees();

auto claim_pool = [&]( const account_id_type& issuer, const asset_id_type& asset_to_claim,
const asset& amount_to_fund, const asset_object& fee_asset )

This comment has been minimized.

Copy link
@pmconrad

pmconrad Feb 21, 2018

Contributor

Makes sense for asset (because that's an object), but not for ..._id_type (as that's basically an int64).

@pmconrad
Copy link
Contributor

left a comment

Looks good, but fee_tests/fee_refund_test is failing for me. Any idea? Unrelated + fixed in develop it seems.

@abitmore abitmore merged commit 6cbf0d1 into hardfork Feb 23, 2018

201806 Protocol Upgrade Release (Hard Fork) automation moved this from In progress to Done Feb 23, 2018

@abitmore abitmore deleted the asset_claim_pool_operation branch Mar 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.