Asset update issuer operation #599

xeroc · 2018-01-25T17:33:03Z

~~WARNING This is a PR without approval from the BTS holders (yet)~~
~~A formal BSIP will follow and a worker will be created for this one.~~
The BSIP
https://github.com/bitshares/bsips/blob/master/bsip-0029.md

I would still like to get feedback on this so that I can present a "final" product to the BTS holders.
For me, this is a playground to learn more about the internal work of BitShares (similar to the other pull request). Doing baby steps here and appreciate all your patients with me!

pmconrad · 2018-01-25T20:04:00Z

libraries/chain/hardfork.d/CORE_199.hf

@@ -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( 1512747600 ))


Please use a HF time in the far future in the PR

pmconrad · 2018-01-25T20:09:47Z

tests/tests/operation_tests.cpp

+         PUSH_TX( db, tx, ~0 );
+      };
+
+      if( db.head_block_time() <= HARDFORK_CORE_199_TIME )


The test is deterministic, so you should know if head_block_time is before or after the hf. No need for the if.

As you wish!

pmconrad · 2018-01-25T20:10:50Z

tests/tests/operation_tests.cpp

+         generate_blocks( HARDFORK_CORE_199_TIME );
+         while( db.head_block_time() <= HARDFORK_CORE_199_TIME )
+         {
+            generate_block();


Use generate_blocks(time_point_sec, skip) to quickly jump to the hf time, skipping over the intermediate blocks. Otherwise these tests take ages.

Oh, I always wondered about that pattern and why the test takes so long.
Thanks for the remark, will change it momentarily

pmconrad · 2018-01-25T20:15:30Z

tests/tests/operation_tests.cpp

+      BOOST_TEST_MESSAGE( "Updating issuer to bob" );
+      update_issuer( test, alice, bob );
+
+   }


Add tests to prove that the active key is not sufficient.

pmconrad · 2018-01-25T20:17:46Z

libraries/chain/asset_evaluator.cpp

+   const asset_object& a = o.asset_to_update(d);
+   auto a_copy = a;
+   a_copy.validate();
+


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

pmconrad · 2018-01-25T20:18:24Z

libraries/chain/asset_evaluator.cpp

@@ -262,6 +262,8 @@ void_result asset_update_evaluator::do_evaluate(const asset_update_operation& o)

   if( o.new_issuer )
   {
+      FC_ASSERT( d.head_block_time() < HARDFORK_CORE_199_TIME,


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

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

pmconrad · 2018-01-25T20:19:39Z

libraries/chain/asset_evaluator.cpp

+      } else
+         FC_ASSERT( backing.get_id() == asset_id_type(),
+                    "May not create a blockchain-controlled market asset which is not backed by CORE.");
+   }


For bonus points, refactor the parts that are identical with asset_update into a separate method.

I am not sure what the best way to do this would be.
Do I simply setup a new function that tests new_issuer, or is some other construct better?

Yes, I'd simply move the code block into a static function.

I fear, this is too difficult for me and I need some guidance for C++. The major issue is that o is of type asset_update_operation or of type asset_update_issuer_operation. I am not sufficiently into templates and/or visitors to produce code that can do that. Could you give me a hint, please?

KISS. ;-)
See latest commit.

abitmore · 2018-01-27T22:14:18Z

Please rebase to targe hardfork branch.

xeroc · 2018-01-30T08:26:39Z

For reference:
The BSIP
https://github.com/bitshares/bsips/blob/master/bsip-0029.md

Forum discussion:
https://bitsharestalk.org/index.php/topic,25896.0.html

…eration

xeroc · 2018-01-30T15:04:01Z

rebase
added cli_wallet command

abitmore · 2018-02-02T16:30:01Z

libraries/chain/asset_evaluator.cpp

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


What's the static for?

That avoids exposing the symbol outside the compilation unit.

xeroc · 2018-02-06T08:51:39Z

@oxarbitrage What's your opinion on this PR?
Please let me know if any other modification is needed!

abitmore · 2018-02-06T17:07:18Z

libraries/chain/asset_evaluator.cpp

-                       "May not create a blockchain-controlled market asset which is not backed by CORE.");
-      }
+      FC_ASSERT( d.head_block_time() < HARDFORK_CORE_199_TIME,
+                 "Since Hardfork #199, updating issuer requires the use of asset_change_issuer_operation.");


asset_change_issuer_operation should be asset_update_issuer_operation

abitmore · 2018-02-06T17:10:00Z

libraries/chain/include/graphene/chain/protocol/asset_ops.hpp

+      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


I think this can be removed.

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.

Correct, not a big deal if keep it.

abitmore · 2018-02-06T17:13:06Z

libraries/chain/asset_evaluator.cpp

+   validate_new_issuer( d, a, o.new_issuer );
+
+   asset_to_update = &a;
+   FC_ASSERT( o.issuer == a.issuer, "", ("o.issuer", o.issuer)("a.issuer", a.issuer) );


The message is missing. The parameters followed won't show.

abitmore · 2018-02-06T17:16:23Z

libraries/wallet/include/graphene/wallet/wallet.hpp

+       *
+       * @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.
+       *                   null if you wish to remain the issuer of the asset


null here doesn't make sense.

abitmore · 2018-02-13T10:49:11Z

tests/tests/operation_tests.cpp

+         return get_account(name);
+      };
+
+      auto update_asset_issuer = [&](asset_object current,


Please change ALL object types in parameters of ALL functions to const references to avoid unnecessary data copy or unintended data modification, like this: const asset_object& current.

abitmore · 2018-02-13T10:51:23Z

tests/tests/operation_tests.cpp

+      generate_blocks( HARDFORK_CORE_199_TIME );
+
+      BOOST_TEST_MESSAGE( "Can't change issuer if not my asset" );
+      GRAPHENE_REQUIRE_THROW( update_issuer( test, bob, alice, bob_active ), fc::exception );


Like mentioned in another PR, references may become invalid after generated a block. Please change them to xxx_id(db).

…and use const references

xeroc · 2018-02-13T12:57:27Z

With make && ./chain_test --run_test=operation_tests/update_uia_issuer, I don't see any failed unit tests here

Random number generator seeded to 1518526204
GRAPHENE_TESTING_GENESIS_TIMESTAMP is 1431700000
Running 1 test case...
3004478ms th_a       db_management.cpp:151         open                 ] Wiping object_database due to missing or wrong version
3004478ms th_a       object_database.cpp:93        wipe                 ] Wiping object database...
3004478ms th_a       object_database.cpp:95        wipe                 ] Done wiping object databse.
3004479ms th_a       object_database.cpp:106       open                 ] Opening object database from /tmp/graphene-tmp/9d11-0b0a-7b86-e1f2 ...
3004479ms th_a       object_database.cpp:111       open                 ] Done opening object database.
3049627ms th_a       db_management.cpp:191         close                ] Rewinding from 3 to 0
3049627ms 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-13T12:50:49"},"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-13T12:50:49"},"format":"","data":{}}]}

*** No errors detected

abitmore · 2018-02-13T13:19:13Z

libraries/wallet/wallet.cpp

-        account_object new_issuer_account = get_account(*new_issuer);
-        new_issuer_account_id = new_issuer_account.id;
-      }
+        FC_THROW("The use of 'new_issuer' is no longer supported. Please use `update_asset_issuer' instead!");


This change need to be done only after the hard fork, otherwise people will be unable to update issuer before the hard fork with this command via this cli_wallet. Perhaps not a big deal though.

abitmore

Unit test code: dereferencing test after generated blocks is not safe.

abitmore · 2018-02-14T16:47:10Z

tests/tests/operation_tests.cpp

+
+      BOOST_TEST_MESSAGE( "Updating issuer to bob" );
+      update_issuer( test, alice_id(db), bob_id(db), alice_owner );
+


Need to test whether the update succeeded. E.G. test(db).issuer == bob_id

And perhaps test other options didn't change.

abitmore · 2018-02-14T16:53:08Z

tests/tests/operation_tests.cpp

+      const auto& test = create_user_issued_asset("UPDATEISSUER", alice_id(db), 0);
+
+      BOOST_TEST_MESSAGE( "can't use this operation before the hardfork" );
+      GRAPHENE_REQUIRE_THROW( update_issuer( test, alice_id(db), bob_id(db), alice_owner), fc::exception );


Need tests for updating with proposals before and after hard fork.

oxarbitrage · 2018-02-20T22:54:21Z

i think this pull can have conflicts with #572

i propose to merge 572 and see if we need to change anything here.

oxarbitrage · 2018-02-23T21:13:51Z

need to resolve conflicts here now #572 was merged.

…er_operation

xeroc · 2018-02-25T15:53:19Z

need to resolve conflicts here now #572 was merged.

Merged hardfork branch into this branch and resolved conflicts.

abitmore · 2018-02-26T10:23:45Z

libraries/chain/include/graphene/chain/protocol/asset_ops.hpp

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

+   /**


This line is redundant.

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

abitmore · 2018-02-26T10:25:36Z

libraries/chain/proposal_evaluator.cpp

@@ -82,6 +82,11 @@ void_result proposal_create_evaluator::do_evaluate(const proposal_create_operati
   { // TODO: remove after HARDFORK_CORE_199_TIME has passed
      graphene::chain::impl::hf_199_visitor hf_199;
      hf_199( o );


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

Sorry for that

abitmore · 2018-02-26T10:26:41Z

libraries/chain/include/graphene/chain/protocol/fee_schedule.hpp

@@ -92,6 +92,19 @@ namespace graphene { namespace chain {
      }
   };

+   class fee_helper<asset_claim_pool_operation> {


Maybe lack of a template<> above this line?

abitmore · 2018-02-26T10:29:57Z

libraries/chain/asset_evaluator.cpp

@@ -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) );


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

abitmore · 2018-02-26T10:30:03Z

libraries/chain/asset_evaluator.cpp

+   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) );


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

abitmore · 2018-02-26T10:33:35Z

libraries/chain/include/graphene/chain/asset_evaluator.hpp

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


Should not remove this class.

abitmore · 2018-02-26T10:37:03Z

libraries/chain/protocol/asset_ops.cpp

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


These two lines have different code styles, looks strange.

oxarbitrage

seems all good to me now. it will be great to have test case for the cli wallets commands. we need to merge this for it #675

pmconrad reviewed Jan 25, 2018

View reviewed changes

abitmore added this to the Future Consensus-Changing Release milestone Jan 25, 2018

abitmore added the hardfork label Jan 25, 2018

xeroc mentioned this pull request Jan 30, 2018

BSIP 29: Require owner key for change of asset-issuer #199

Closed

xeroc added 4 commits January 30, 2018 15:22

Disable use of new_issuer after hardfork #199

0192a02

Implement functionality of asset_update_issuer_operation

75ece83

Add hf_199 visitor and implement unittests for asset_update_issuer_op…

4e9f6e9

…eration

improved unittests and fixed hardfork conditions

05bd614

xeroc force-pushed the asset_update_issuer_operation branch from c44f25d to 05bd614 Compare January 30, 2018 14:43

xeroc changed the base branch from develop to hardfork January 30, 2018 14:43

[wallet] add update_asset_issuer call in the cli_wallet

08b1e40

Refactored some common code into new function

29a3f68

abitmore reviewed Feb 2, 2018

View reviewed changes

abitmore requested changes Feb 6, 2018

View reviewed changes

abitmore modified the milestones: Future Consensus-Changing Release, 201803 - Consensus Changing Release Feb 8, 2018

xeroc added 2 commits February 9, 2018 13:52

fix assert comment

ff4a35b

fix documentation of update_asset_issuer call in the wallet

f5c2e0a

Fix assert message

4a91978

abitmore approved these changes Feb 12, 2018

View reviewed changes

xeroc mentioned this pull request Feb 13, 2018

New operation asset_update_issuer_operation, breaking change! bitshares/bitshares-ui#1155

Closed

abitmore requested changes Feb 13, 2018

View reviewed changes

[tests] properly reload objects after new blocks have been generated …

2cfef3e

…and use const references

abitmore reviewed Feb 13, 2018

View reviewed changes

abitmore approved these changes Feb 13, 2018

View reviewed changes

[wallet] ensure that asset_update works until the hard fork

53bb08b

abitmore requested changes Feb 14, 2018

View reviewed changes

abitmore reviewed Feb 14, 2018

View reviewed changes

xeroc added 2 commits February 15, 2018 17:33

[tests] add test to verify new issuer, also modify to reload objects

76de8f4

[tests] add unit test for the hf_xxx_visitor via proposals

d2f67c3

abitmore approved these changes Feb 16, 2018

View reviewed changes

Merge remote-tracking branch 'origin/hardfork' into asset_update_issu…

8e49b2c

…er_operation

abitmore requested changes Feb 26, 2018

View reviewed changes

abitmore mentioned this pull request Feb 27, 2018

Virtual operations should be excluded from transactions #588

Closed

fix merge fuckups

e2be061

abitmore approved these changes Feb 27, 2018

View reviewed changes

oxarbitrage approved these changes Mar 6, 2018

View reviewed changes

abitmore merged commit 1944754 into hardfork Mar 7, 2018

oxarbitrage mentioned this pull request Mar 8, 2018

Issue620 - BSIP37: Allow new asset name to end with a number #705

Merged

abitmore deleted the asset_update_issuer_operation branch March 17, 2018 21:34


		BOOST_TEST_MESSAGE( "Updating issuer to bob" );
		update_issuer( test, alice_id(db), bob_id(db), alice_owner );

Asset update issuer operation #599

Asset update issuer operation #599

Conversation

xeroc commented Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore commented Jan 27, 2018

xeroc commented Jan 30, 2018

xeroc commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xeroc commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xeroc commented Feb 13, 2018

Choose a reason for hiding this comment

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxarbitrage commented Feb 20, 2018

oxarbitrage commented Feb 23, 2018

xeroc commented Feb 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxarbitrage left a comment

Choose a reason for hiding this comment

xeroc commented Jan 25, 2018 •

edited

Loading