BSIP 40: Code review, round 2

Author: nathanielhourt

libraries/chain/custom_authority_evaluator.cpp

@@ -48,7 +48,7 @@ void_result custom_authority_create_evaluator::do_evaluate(const custom_authorit
    bool operation_forked_in = hardfork_visitor(now).visit((operation::tag_type)op.operation_type.value);
    FC_ASSERT(operation_forked_in, "Cannot create custom authority for operation which is not valid yet");
 
-   auto restriction_count = std::for_each(op.restrictions.begin(), op.restrictions.end(), restriction::adder()).sum;
+   auto restriction_count = restriction::restriction_count(op.restrictions);
    FC_ASSERT(restriction_count <= config->max_custom_authority_restrictions,
              "Custom authority has more than the maximum number of restrictions");
 
@@ -137,8 +137,7 @@ void_result custom_authority_update_evaluator::do_evaluate(const custom_authorit
    for (const auto& restriction_pair : old_object->restrictions)
       if (op.restrictions_to_remove.count(restriction_pair.first) == 0)
          restriction_count += restriction_pair.second.restriction_count();
-   restriction_count += std::for_each(op.restrictions_to_add.begin(), op.restrictions_to_add.end(),
-                                      restriction::adder()).sum;
+   restriction_count += restriction::restriction_count(op.restrictions_to_add);
    // Check restriction count against limit
    FC_ASSERT(restriction_count <= config->max_custom_authority_restrictions,
              "Cannot update custom authority: updated authority would exceed the maximum number of restrictions");

libraries/chain/proposal_evaluator.cpp

@@ -95,8 +95,7 @@ struct proposal_operation_hardfork_visitor
    }
    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");
+         FC_ASSERT(!op.new_parameters.extensions.value.updatable_htlc_options.valid(), "Unable to set HTLC options before hardfork 1468");
          FC_ASSERT(!op.new_parameters.current_fees->exists<htlc_create_operation>());
          FC_ASSERT(!op.new_parameters.current_fees->exists<htlc_redeem_operation>());
          FC_ASSERT(!op.new_parameters.current_fees->exists<htlc_extend_operation>());

libraries/protocol/CMakeLists.txt

@@ -16,6 +16,7 @@ list(APPEND SOURCES account.cpp
                     asset.cpp
                     authority.cpp
                     special_authority.cpp
+                    restriction.cpp
                     custom_authority.cpp
                     committee_member.cpp
                     custom.cpp

libraries/protocol/include/graphene/protocol/restriction.hpp

@@ -108,25 +108,8 @@ struct restriction {
    restriction(const unsigned_int& member_index, function_type type, const argument_type& argument)
       : member_index(member_index), restriction_type(type), argument(argument) {}
 
-   struct adder {
-      size_t sum = 0;
-      void operator()(const restriction& r) { sum += r.restriction_count(); }
-      void operator()(const vector<restriction>& r) { sum += std::for_each(r.begin(), r.end(), adder()).sum; }
-   };
-
-   size_t restriction_count() const {
-      if (argument.is_type<vector<restriction>>()) {
-         const vector<restriction>& rs = argument.get<vector<restriction>>();
-         return 1 + std::for_each(rs.begin(), rs.end(), adder()).sum;
-      } else if (argument.is_type<vector<vector<restriction>>>()) {
-         const vector<vector<restriction>>& rs = argument.get<vector<vector<restriction>>>();
-         return 1 + std::for_each(rs.begin(), rs.end(), adder()).sum;
-      } else if (argument.is_type<variant_assert_argument_type>()) {
-         const variant_assert_argument_type& arg = argument.get<variant_assert_argument_type>();
-         return 1 + std::for_each(arg.second.begin(), arg.second.end(), adder()).sum;
-      }
-      return 1;
-   }
+   static size_t restriction_count(const vector<restriction>& restrictions);
+   size_t restriction_count() const;
 };
 
 } } // graphene::protocol

libraries/protocol/restriction.cpp

@@ -0,0 +1,53 @@
+/*
+ * Copyright (c) 2019 Contributors.
+ *
+ * The MIT License
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <graphene/protocol/restriction.hpp>
+
+namespace graphene { namespace protocol {
+
+struct adder {
+   size_t sum = 0;
+   void operator()(const restriction& r) { sum += r.restriction_count(); }
+   void operator()(const vector<restriction>& r) { sum += std::for_each(r.begin(), r.end(), adder()).sum; }
+};
+
+size_t restriction::restriction_count(const vector<restriction>& restrictions) {
+   return std::for_each(restrictions.begin(), restrictions.end(), adder()).sum;
+}
+
+size_t restriction::restriction_count() const {
+   if (argument.is_type<vector<restriction>>()) {
+      const vector<restriction>& rs = argument.get<vector<restriction>>();
+      return 1 + std::for_each(rs.begin(), rs.end(), adder()).sum;
+   } else if (argument.is_type<vector<vector<restriction>>>()) {
+      const vector<vector<restriction>>& rs = argument.get<vector<vector<restriction>>>();
+      return 1 + std::for_each(rs.begin(), rs.end(), adder()).sum;
+   } else if (argument.is_type<variant_assert_argument_type>()) {
+      const variant_assert_argument_type& arg = argument.get<variant_assert_argument_type>();
+      return 1 + std::for_each(arg.second.begin(), arg.second.end(), adder()).sum;
+   }
+   return 1;
+}
+
+} } // namespace graphene::protocol

tests/tests/custom_authority_tests.cpp

@@ -52,9 +52,6 @@ BOOST_FIXTURE_TEST_SUITE(custom_authority_tests, database_fixture)
 
 #define FUNC(TYPE) BOOST_PP_CAT(restriction::func_, TYPE)
 
-size_t restriction_count(const vector<restriction>& rs) {
-   return std::for_each(rs.begin(), rs.end(), restriction::adder()).sum;
-}
 template<typename Object>
 unsigned_int member_index(string name) {
    unsigned_int index;
@@ -109,7 +106,7 @@ BOOST_AUTO_TEST_CASE(restriction_predicate_tests) { try {
    //    "extensions": []
    //  }
    //]
-   BOOST_CHECK_EQUAL(restriction_count(restrictions), 1);
+   BOOST_CHECK_EQUAL(restriction::restriction_count(restrictions), 1);
    BOOST_CHECK(get_restriction_predicate(restrictions, operation::tag<transfer_operation>::value)(transfer)
                .rejection_path.size() == 2);
    // Index 0 (the outer-most) rejection path refers to the first and only restriction
@@ -226,7 +223,7 @@ BOOST_AUTO_TEST_CASE(restriction_predicate_tests) { try {
    //  }
    //]
 
-   BOOST_CHECK_EQUAL(restriction_count(restrictions), 2);
+   BOOST_CHECK_EQUAL(restriction::restriction_count(restrictions), 2);
    //////
    // Check the transfer operation that pays the fee with Asset ID 0 against the restriction.
    // This should violate the restriction.
@@ -280,7 +277,7 @@ BOOST_AUTO_TEST_CASE(restriction_predicate_tests) { try {
    //    "extensions": []
    //  }
    //]
-   BOOST_CHECK_EQUAL(restriction_count(restrictions), 3);
+   BOOST_CHECK_EQUAL(restriction::restriction_count(restrictions), 3);
 
    //////
    // Create a transfer operation that authorizes transfer to Account ID 12
@@ -338,7 +335,7 @@ BOOST_AUTO_TEST_CASE(restriction_predicate_tests) { try {
    //    "extensions": []
    //  }
    //]
-   BOOST_CHECK_EQUAL(restriction_count(restrictions), 2);
+   BOOST_CHECK_EQUAL(restriction::restriction_count(restrictions), 2);
    auto predicate = get_restriction_predicate(restrictions, operation::tag<account_update_operation>::value);
 
    //////
@@ -453,7 +450,7 @@ BOOST_AUTO_TEST_CASE(restriction_predicate_tests) { try {
          //    "extensions": []
          //  }
          //]
-         BOOST_CHECK_EQUAL(restriction_count(or_restrictions), 3);
+         BOOST_CHECK_EQUAL(restriction::restriction_count(or_restrictions), 3);
          auto predicate = get_restriction_predicate(or_restrictions, operation::tag<transfer_operation>::value);
 
          //////
@@ -570,7 +567,7 @@ BOOST_AUTO_TEST_CASE(custom_auths) { try {
    //    "extensions": []
    //  }
    //]
-   BOOST_CHECK_EQUAL(restriction_count(op.restrictions), 3);
+   BOOST_CHECK_EQUAL(restriction::restriction_count(op.restrictions), 3);
 
 
    //////
@@ -1643,7 +1640,7 @@ BOOST_AUTO_TEST_CASE(custom_auths) { try {
          //    "extensions": []
          //  }
          //]
-         BOOST_CHECK_EQUAL(restriction_count(op.restrictions), 3);
+         BOOST_CHECK_EQUAL(restriction::restriction_count(op.restrictions), 3);
 
          // Publish the new custom authority
          trx.clear();