hyperledger-iroha / iroha-dco

Iroha - A simple, decentralized ledger
http://iroha.tech
Apache License 2.0
988 stars 297 forks source link

Brave new permission #1099

Closed l4l closed 6 years ago

l4l commented 6 years ago

So far permissions are still used from the old model and represented via hard-coded strings: https://github.com/hyperledger/iroha/blob/c3b1a743e977de8a517b754eb727d7b9b997545d/irohad/model/permissions.hpp#L34-L48

And its usage (e.g in itf) is not really good as it might be: https://github.com/hyperledger/iroha/blob/c3b1a743e977de8a517b754eb727d7b9b997545d/test/framework/integration_framework/integration_test_framework.cpp#L80-L83

Generally this way has code duplication (variable name == its value) and bad typing (in string might be everything). Maybe there is also performance/memory issues, but most probably they are insignificant

So far I have 3 ideas for solving it issue (feel free to propose new one). The first two use additional strings as it is, the last one is prefer depend on the protobuf (thus less transport-agnostic, but more stable). All the ways describe how define enum Permission, and related toString & fromString functions. Imo that should be enough for replacing (am I missing smth?).

p.s code may cause compile-errors, consider as pseudo-code

  1. Macros. Yes, they are considered evilish but the solution is the most compact and has no code duplication at all. New permission -> 1 LOC
#define __PERM_SWITCH_TOSTR(r, _, elem) case Permission::elem: return ##elem;
#define __PERM_SWITCH_FROMSTR(r, _, elem) {##elem, Permission::elem}
#define PERM_DEF(perm_list)                                         \
  enum class Permission {                                          \
    perm_list                                                      \
    , NONE                                                         \
  };                                                               \
  inline const std::string toString(Permission p) {                \
    switch (p) {                                                   \
      BOOST_PP_SEQ_FOR_EACH(__PERM_SWITCH_TOSTR, _, perm_list)     \
    }                                                              \
    return "undefined";                                             \
  }                                                                \
  inline Permission fromString(const std::string s) {              \
    static std::map<std::string, Permission> m {                   \
      BOOST_PP_SEQ_FOR_EACH(__PERM_SWITCH_FROMSTR, _, perm_list)   \
    };                                                             \
  }

PERM_DEF((can_receive)
         (can_transfer)
         (can_create_domain)
         ...)
  1. Constexpr maps, a lot of code duplication but compile-time (-> prob less error-prone), need to write function str -> enum (idk how to do it via templates). It is roughly same to the case enum+pair of methods, I don't show this case, because constexpr-maps is a bit better (for toString) New permission -> 4 LOC (enum + PermMap::s (x2) + fromString's map)
enum class Permission {
  can_receive,
  can_transfer,
  can_create_domain,
};

template<Permission>
class PermMap {
  static const std::string s;
};

template<>
const std::string PermMap<Permission::can_receive>::s = "can_receive";

inline const std::string toString(Permission p) {
  return PermMap<p>::s;
}

inline Permission fromString(const std::string s) {
  static const std::map<std::string, Permission> m {
    {"can_receive", Permission::can_receive},
    {"can_transfer", Permission::can_transfer},
    {"can_create_domain", Permission::can_create_domain},
    ...
  };
  return m[s];
}
  1. Depend on the protobuf, the most elegant way. The only problem that it highly depend on the transport so imo it is a bad thing to use. Also protobuf uses enum (NOT enum class, so weak typing might be a huge problem in future) New permission -> 0 LOC Though protobuf updates may need some fixes
using Permission = iroha::protocol::RolePermission;

inline const std::string toString(Permission p) {
  return iroha::protocol::RolePermission_Name(p);
}

inline Permission fromString(const std::string &s) {
  Permission p;
  iroha::protocol::RolePermission_Parse(s, &p);
  return p;
}
nickaleks commented 6 years ago

Please check https://github.com/hyperledger/iroha/issues/1045

l4l commented 6 years ago

Moved, thx