BSIP 40: Finish writing operations

nathanielhourt · nathanielhourt · commit 91bbaccbe2f1 · 2019-07-19T23:23:23.000-05:00
Notably, I made an overhaul to the fee logic here. The old logic was
incredibly complex and had weird multiply-by-zero edge cases to get a
free fee... I dropped the whole thing and replaced it with a simple
price per byte, which should have a fairly similar effect without
segfaulting anyone's brain.
diff --git a/libraries/protocol/CMakeLists.txt b/libraries/protocol/CMakeLists.txt
@@ -17,7 +17,6 @@ list(APPEND SOURCES account.cpp
                     authority.cpp
                     special_authority.cpp
                     custom_authority.cpp
-                    restriction.cpp
                     restriction_predicate.cpp
                     committee_member.cpp
                     custom.cpp
diff --git a/libraries/protocol/custom_authority.cpp b/libraries/protocol/custom_authority.cpp
@@ -23,100 +23,59 @@
  */
 #include <graphene/protocol/custom_authority.hpp>
 #include <graphene/protocol/operations.hpp>
+#include <graphene/protocol/restriction_predicate.hpp>
 
 namespace graphene { namespace protocol {
 
-share_type custom_authority_create_operation::calculate_fee( const fee_parameters_type& k )const
-{
+share_type custom_authority_create_operation::calculate_fee(const fee_parameters_type& k)const {
    share_type core_fee_required = k.basic_fee;
-
-   if( enabled )
-   {
-      share_type unit_fee = k.price_per_k_unit;
-      unit_fee *= (valid_to - valid_from).to_seconds();
-      unit_fee *= auth.num_auths();
-      uint64_t restriction_units = 0;
-      for( const auto& r : restrictions )
-      {
-         restriction_units += r.get_units();
-      }
-      unit_fee *= restriction_units;
-      unit_fee /= 1000;
-      core_fee_required += unit_fee;
-   }
-
+   core_fee_required += k.price_per_byte * (fc::raw::pack_size(restrictions) + fc::raw::pack_size(auth));
    return core_fee_required;
 }
 
-void custom_authority_create_operation::validate()const
-{
-   FC_ASSERT( fee.amount >= 0, "Fee amount can not be negative" );
+void custom_authority_create_operation::validate()const {
+   FC_ASSERT(fee.amount >= 0, "Fee amount can not be negative");
 
-   FC_ASSERT( account != GRAPHENE_TEMP_ACCOUNT
-              && account != GRAPHENE_COMMITTEE_ACCOUNT
-              && account != GRAPHENE_WITNESS_ACCOUNT
-              && account != GRAPHENE_RELAXED_COMMITTEE_ACCOUNT,
-              "Can not create custom authority for special accounts" );
+   FC_ASSERT(account != GRAPHENE_TEMP_ACCOUNT
+             && account != GRAPHENE_COMMITTEE_ACCOUNT
+             && account != GRAPHENE_WITNESS_ACCOUNT
+             && account != GRAPHENE_RELAXED_COMMITTEE_ACCOUNT,
+             "Can not create custom authority for special accounts");
 
-   FC_ASSERT( valid_from < valid_to, "valid_from must be earlier than valid_to" );
+   FC_ASSERT(valid_from < valid_to, "valid_from must be earlier than valid_to");
 
-   // Note: when adding new operation with hard fork, need to check more strictly in evaluator
-   // TODO add code in evaluator
-   FC_ASSERT( operation_type.value < operation::count(), "operation_type is too large" );
+   // Note: The authentication authority can be empty, but it cannot be impossible to satisify. Disable the authority
+   // using the `enabled` boolean rather than setting an impossible authority.
 
-   // Note: allow auths to be empty
-   //FC_ASSERT( auth.num_auths() > 0, "Can not set empty auth" );
-   FC_ASSERT( auth.address_auths.size() == 0, "Address auth is not supported" );
-   // Note: allow auths to be impossible
-   //FC_ASSERT( !auth.is_impossible(), "cannot use an imposible authority threshold" );
+   FC_ASSERT(auth.address_auths.size() == 0, "Address authorities are not supported");
+   FC_ASSERT(!auth.is_impossible(), "Cannot use an imposible authority threshold");
 
-   // Note: allow restrictions to be empty
-   for( const auto& r: restrictions )
-   {
-      // recursively validate member index and argument type
-      r.validate( operation_type );
-   }
+   // Validate restrictions by constructing a predicate for them; this throws if restrictions aren't valid
+   get_restriction_predicate(restrictions, operation_type);
 }
 
-share_type custom_authority_update_operation::calculate_fee( const fee_parameters_type& k )const
-{
+share_type custom_authority_update_operation::calculate_fee(const fee_parameters_type& k)const {
    share_type core_fee_required = k.basic_fee;
-
-   share_type unit_fee = k.price_per_k_unit;
-   unit_fee *= delta_units;
-   unit_fee /= 1000;
-
-   return core_fee_required + unit_fee;
+   core_fee_required += k.price_per_byte * fc::raw::pack_size(restrictions_to_add);
+   if (new_auth)
+      core_fee_required += k.price_per_byte * fc::raw::pack_size(*new_auth);
+   return core_fee_required;
 }
 
-void custom_authority_update_operation::validate()const
-{
-   FC_ASSERT( fee.amount >= 0, "Fee amount can not be negative" );
-
-   FC_ASSERT( account != GRAPHENE_TEMP_ACCOUNT
-              && account != GRAPHENE_COMMITTEE_ACCOUNT
-              && account != GRAPHENE_WITNESS_ACCOUNT
-              && account != GRAPHENE_RELAXED_COMMITTEE_ACCOUNT,
-              "Can not create custom authority for special accounts" );
-/*
-   FC_ASSERT( valid_from < valid_to, "valid_from must be earlier than valid_to" );
-   // Note: when adding new operation with hard fork, need to check more strictly in evaluator
-   // TODO add code in evaluator
-   FC_ASSERT( operation_type < operation::count(), "operation type too large" );
-   FC_ASSERT( auth.num_auths() > 0, "Can not set empty auth" );
-   FC_ASSERT( auth.address_auths.size() == 0, "Address auth is not supported" );
-   //FC_ASSERT( !auth.is_impossible(), "cannot use an imposible authority threshold" );
-   // Note: allow restrictions to be empty
-   for( const auto& r : restrictions )
-   {
-      // TODO recursively validate member index and argument type
-      r.validate( operation_type );
+void custom_authority_update_operation::validate()const {
+   FC_ASSERT(fee.amount >= 0, "Fee amount can not be negative");
+
+   FC_ASSERT(account != GRAPHENE_TEMP_ACCOUNT
+             && account != GRAPHENE_COMMITTEE_ACCOUNT
+             && account != GRAPHENE_WITNESS_ACCOUNT
+             && account != GRAPHENE_RELAXED_COMMITTEE_ACCOUNT,
+             "Can not create custom authority for special accounts");
+   if (new_valid_from && new_valid_to)
+      FC_ASSERT(*new_valid_from < new_valid_to, "valid_from must be earlier than valid_to");
+   if (new_auth) {
+      FC_ASSERT(!new_auth->is_impossible(), "Cannot use an impossible authority threshold");
+      FC_ASSERT(new_auth->address_auths.size() == 0, "Address auth is not supported");
    }
-*/
-}
-
-void custom_authority_delete_operation::validate()const
-{
 }
 
 } } // graphene::protocol
diff --git a/libraries/protocol/include/graphene/protocol/authority.hpp b/libraries/protocol/include/graphene/protocol/authority.hpp
@@ -108,7 +108,7 @@ namespace graphene { namespace protocol {
                 (a.address_auths == b.address_auths); 
       }
       uint32_t num_auths()const { return account_auths.size() + key_auths.size() + address_auths.size(); }
-      void     clear() { account_auths.clear(); key_auths.clear(); }
+      void     clear() { account_auths.clear(); key_auths.clear(); address_auths.clear(); weight_threshold = 0; }
 
       static authority null_authority()
       {
diff --git a/libraries/protocol/include/graphene/protocol/custom_authority.hpp b/libraries/protocol/include/graphene/protocol/custom_authority.hpp
@@ -33,87 +33,104 @@ namespace graphene { namespace protocol {
     * @brief Create a new custom authority
     * @ingroup operations
     */
-   struct custom_authority_create_operation : public base_operation
-   {
-      struct fee_parameters_type
-      {
-         uint64_t basic_fee        = GRAPHENE_BLOCKCHAIN_PRECISION;
-         uint32_t price_per_k_unit = 100; ///< units = valid seconds * items in auth * items in restrictions
+   struct custom_authority_create_operation : public base_operation {
+      struct fee_parameters_type {
+         uint64_t basic_fee = GRAPHENE_BLOCKCHAIN_PRECISION;
+         uint32_t price_per_byte = GRAPHENE_BLOCKCHAIN_PRECISION / 10;
       };
 
-      asset                           fee; // TODO: defer fee to expiration / update / removal ?
-      account_id_type                 account;
-      uint32_t                        custom_id;
-      bool                            enabled;
-      time_point_sec                  valid_from;
-      time_point_sec                  valid_to;
-      unsigned_int                    operation_type;
-      authority                       auth;
-      vector<restriction>             restrictions;
+      /// Operation fee
+      asset fee;
+      /// Account which is setting the custom authority; also pays the fee
+      account_id_type account;
+      /// Whether the custom authority is enabled or not
+      bool enabled;
+      /// Date when custom authority becomes active
+      time_point_sec valid_from;
+      /// Expiration date for custom authority
+      time_point_sec valid_to;
+      /// Tag of the operation this custom authority can authorize
+      unsigned_int operation_type;
+      /// Authentication requirements for the custom authority
+      authority auth;
+      /// Restrictions on operations this custom authority can authenticate
+      vector<restriction> restrictions;
 
       extensions_type extensions;
 
       account_id_type fee_payer()const { return account; }
-      void            validate()const;
-      share_type      calculate_fee(const fee_parameters_type& k)const;
+      void validate()const;
+      share_type calculate_fee(const fee_parameters_type& k)const;
    };
 
    /**
     * @brief Update a custom authority
     * @ingroup operations
     */
-   struct custom_authority_update_operation : public base_operation
-   {
-      struct fee_parameters_type
-      {
-         uint64_t basic_fee        = GRAPHENE_BLOCKCHAIN_PRECISION;
-         uint32_t price_per_k_unit = 100; ///< units = valid seconds * items in auth * items in restrictions
+   struct custom_authority_update_operation : public base_operation {
+      struct fee_parameters_type {
+         uint64_t basic_fee = GRAPHENE_BLOCKCHAIN_PRECISION;
+         uint32_t price_per_byte = GRAPHENE_BLOCKCHAIN_PRECISION / 10;
       };
 
-      asset             fee;
-      account_id_type   account;
-      uint64_t          delta_units; // to calculate fee, it will be validated in evaluator
-                                     // Note: if start was in the past, when updating, used fee should be deducted
+      /// Operation fee
+      asset fee;
+      /// Account which owns the custom authority to update; also pays the fee
+      account_id_type account;
+      /// ID of the custom authority to update
+      custom_authority_id_type authority_to_update;
+      /// Change to whether the custom authority is enabled or not
+      optional<bool> new_enabled;
+      /// Change to the custom authority begin date
+      optional<time_point_sec> new_valid_from;
+      /// Change to the custom authority expiration date
+      optional<time_point_sec> new_valid_to;
+      /// Change to the authentication for the custom authority
+      optional<authority> new_auth;
+      /// Set of indexes of restrictions to remove
+      flat_set<uint16_t> restrictions_to_remove;
+      /// Vector of new restrictions; will be appended to the old list
+      vector<restriction> restrictions_to_add;
+
+      extensions_type extensions;
 
       account_id_type fee_payer()const { return account; }
-      void            validate()const;
-      share_type      calculate_fee(const fee_parameters_type& k)const;
+      void validate()const;
+      share_type calculate_fee(const fee_parameters_type& k)const;
    };
 
 
    /**
     * @brief Delete a custom authority
     * @ingroup operations
     */
-   struct custom_authority_delete_operation : public base_operation
-   {
+   struct custom_authority_delete_operation : public base_operation {
       struct fee_parameters_type { uint64_t fee =  GRAPHENE_BLOCKCHAIN_PRECISION; };
 
-      asset             fee;
-      account_id_type   account;
+      /// Operation fee
+      asset fee;
+      /// Account which owns the custom authority to update; also pays the fee
+      account_id_type account;
+      /// ID of the custom authority to delete
+      custom_authority_id_type authority_to_delete;
+
+      extensions_type extensions;
 
       account_id_type fee_payer()const { return account; }
-      void            validate()const;
+      void validate()const {}
+      share_type calculate_fee(const fee_parameters_type& k)const { return k.fee; }
    };
 
 } } // graphene::protocol
 
-FC_REFLECT( graphene::protocol::custom_authority_create_operation::fee_parameters_type, (basic_fee)(price_per_k_unit) )
-FC_REFLECT( graphene::protocol::custom_authority_update_operation::fee_parameters_type, (basic_fee)(price_per_k_unit) )
-FC_REFLECT( graphene::protocol::custom_authority_delete_operation::fee_parameters_type, (fee) )
-
-FC_REFLECT( graphene::protocol::custom_authority_create_operation,
-            (fee)
-            (account)
-            (custom_id)
-            (enabled)
-            (valid_from)
-            (valid_to)
-            (operation_type)
-            (auth)
-            (restrictions)
-            (extensions)
-          )
-
-FC_REFLECT( graphene::protocol::custom_authority_update_operation, (fee)(account) )
-FC_REFLECT( graphene::protocol::custom_authority_delete_operation, (fee)(account) )
+FC_REFLECT(graphene::protocol::custom_authority_create_operation::fee_parameters_type, (basic_fee)(price_per_byte))
+FC_REFLECT(graphene::protocol::custom_authority_update_operation::fee_parameters_type, (basic_fee)(price_per_byte))
+FC_REFLECT(graphene::protocol::custom_authority_delete_operation::fee_parameters_type, (fee))
+
+FC_REFLECT(graphene::protocol::custom_authority_create_operation,
+           (fee)(account)(enabled)(valid_from)(valid_to)(operation_type)(auth)(restrictions)(extensions))
+
+FC_REFLECT(graphene::protocol::custom_authority_update_operation,
+           (fee)(account)(authority_to_update)(new_enabled)(new_valid_from)
+           (new_valid_to)(new_auth)(restrictions_to_remove)(restrictions_to_add)(extensions))
+FC_REFLECT(graphene::protocol::custom_authority_delete_operation, (fee)(account)(authority_to_delete)(extensions))
diff --git a/libraries/protocol/include/graphene/protocol/restriction.hpp b/libraries/protocol/include/graphene/protocol/restriction.hpp
@@ -101,11 +101,6 @@ struct restriction {
     restriction() = default;
     restriction(const string& member_name, function_type type, const argument_type& argument)
         : member_name(member_name), restriction_type(type), argument(argument) {}
-
-    uint64_t get_units()const;
-
-    /// Validates the restriction with given operation type, to be called by an operation validator
-    void validate( unsigned_int op_type )const;
 };
 
 } } // graphene::protocol
diff --git a/libraries/protocol/include/graphene/protocol/types.hpp b/libraries/protocol/include/graphene/protocol/types.hpp
@@ -260,7 +260,8 @@ GRAPHENE_DEFINE_IDS(protocol, protocol_ids, /*protocol objects are not prefixed*
                     (vesting_balance)
                     (worker)
                     (balance)
-                    (htlc))
+                    (htlc)
+                    (custom_authority))
 
 FC_REFLECT(graphene::protocol::public_key_type, (key_data))
 FC_REFLECT(graphene::protocol::public_key_type::binary_key, (data)(check))
diff --git a/libraries/protocol/restriction.cpp b/libraries/protocol/restriction.cpp
diff --git a/libraries/protocol/restriction_predicate.cpp b/libraries/protocol/restriction_predicate.cpp

Original file line number	Diff line number	Diff line change
`@@ -108,7 +108,7 @@ namespace graphene { namespace protocol {`
`108`	`108`	`(a.address_auths == b.address_auths);`
`109`	`109`	`}`
`110`	`110`	`uint32_t num_auths()const { return account_auths.size() + key_auths.size() + address_auths.size(); }`
`111`		`- void clear() { account_auths.clear(); key_auths.clear(); }`
	`111`	`+ void clear() { account_auths.clear(); key_auths.clear(); address_auths.clear(); weight_threshold = 0; }`
`112`	`112`
`113`	`113`	`static authority null_authority()`
`114`	`114`	`{`