KEP-1552 Get Peer Info creates correct ESR request data #341
Conversation
rnistuk seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
utils/esr_peer_info.cpp
Outdated
std::string result{parameter}; | ||
result.insert(result.size(), PADDING_REQUIRED, '0'); | ||
return result; | ||
static const size_t REQUIRED_SIZE_MULTIPLE{64}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please stop using statics... move to unnamed namespaces.
You incur a performance hit for every call of a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
utils/esr_peer_info.cpp
Outdated
const std::string | ||
data_string_for_get_peer_info(const std::string &swarm_id, const std::string &peer_id) | ||
{ | ||
static const off_t PARAMETER_OFFSET{64}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to unnamed namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All statics moved to anonymous namespace where appropriate
utils/esr_peer_info.cpp
Outdated
// TODO: replace this with a function that uses the ABI to create the request data | ||
// data_string_for_get_peer_info has been moved out of the anonymous namespace to make it possible to | ||
// unit test directly. | ||
const std::string | ||
data_string_for_get_peer_info(const std::string &swarm_id, const std::string &peer_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix coding standard
utils/esr_peer_info.cpp
Outdated
static const auto PEER_INFO_ABI{str_to_json(GET_PEER_INFO_ABI)}; | ||
static const auto GET_PEER_INFO_SIGNATURE{PEER_INFO_ABI["signature"].asCString() + 2}; | ||
static const auto GET_PEER_INFO_SIGNATURE{PEER_INFO_ABI["signature"].asString()}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
utils/esr_peer_info.cpp
Outdated
@@ -394,8 +394,8 @@ namespace | |||
const std::string | |||
data_string_for_get_peers(const std::string &swarm_id) | |||
{ | |||
static const auto NODE_LIST_ABI = str_to_json(GET_NODE_LIST_ABI); | |||
static const auto GET_PEERS_ADDRESS{NODE_LIST_ABI["signature"].asCString() + 2}; // 0x46e76d8b -> 46e76d8b | |||
const auto NODE_LIST_ABI = str_to_json(GET_NODE_LIST_ABI); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you were better as a static here... why don;t you add a json version to the consts instead of converting it
utils/esr_peer_info.cpp
Outdated
static const off_t PARAMETER_OFFSET{64}; | ||
static const auto PEER_INFO_ABI{str_to_json(GET_PEER_INFO_ABI)}; | ||
static const auto GET_PEER_INFO_SIGNATURE{PEER_INFO_ABI["signature"].asString()}; | ||
const auto PEER_INFO_ABI{str_to_json(GET_PEER_INFO_ABI)}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
No description provided.