Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sync code of cpp-sdk #3447

Open
wants to merge 2 commits into
base: feature-3.4.0
Choose a base branch
from

Conversation

LucasLi1024
Copy link
Collaborator

No description provided.

@LucasLi1024 LucasLi1024 force-pushed the feature-3.4.0 branch 4 times, most recently from be699c8 to c70b871 Compare March 10, 2023 03:34
@@ -45,9 +45,6 @@ std::shared_ptr<bytes> HsmSM2Crypto::sign(
auto pwdConstPtr =
std::make_shared<bytes>(hsmKeyPair.password().begin(), hsmKeyPair.password().end());
key = Key(hsmKeyPair.keyIndex(), pwdConstPtr);
CRYPTO_LOG(DEBUG) << "[HSMSignature::key] is internal key "
<< LOG_KV("keyIndex", key.identifier())
<< LOG_KV("password", hsmKeyPair.password());
}
else
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the code review.
First of all, we can see that there is a block of code that has been commented out. This code is logging the key index and password when an internal key is used. We can use this code to track any errors that may occur when the internal key is used, which is a good practice.

The next thing we can notice is that there is no error checking for the key index or password. It would be a good idea to add an if statement to check that the key index and password are valid before proceeding.

Finally, we can see that the shared pointer is being created with the key index and password directly from the hsmKeyPair object. This could potentially lead to security issues, as the password may be exposed in memory. To prevent this, we should use a secure string library to store the password in memory, and then use the secure string library methods to access the password when needed.

Overall, the code looks good, but some minor improvements can be made to increase security and improve the logging capabilities.

@@ -42,7 +43,8 @@ HsmSM2KeyPair::HsmSM2KeyPair(std::string _libPath, SecretPtr _secretKey) : HsmSM
m_publicKey = priToPub(_secretKey);
}

HsmSM2KeyPair::HsmSM2KeyPair(std::string _libPath, unsigned int _keyIndex, std::string _password)
HsmSM2KeyPair::HsmSM2KeyPair(
const std::string& _libPath, unsigned int _keyIndex, const std::string& _password)
: HsmSM2KeyPair(_libPath)
{
m_keyIndex = _keyIndex;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the code review

First, I would suggest that you make sure that all of your parameters are passed by reference (using '&') rather than by value. This helps to ensure that any changes made to the parameters do not affect the original variable. Additionally, I would suggest making sure you are using const when appropriate to guarantee that any changes are not made to a parameter.

Another thing I noticed is that the code does not use the SDR_OK constant that is defined earlier in the code. It is important to use constants to improve readability and maintainability of the code.

Finally, I suggest adding comments to your code so that other developers can better understand what the code is doing. Additionally, this helps to keep track of changes that have been made over time.

unsigned int keyIndex() const { return m_keyIndex; }
void setKeyIndex(unsigned int _keyIndex)
{
m_keyIndex = _keyIndex;
m_isInternalKey = true;
}
const std::string& password() const { return m_password; }
void setPassword(std::string _password)
void setPassword(const std::string& _password)
{
m_password = _password;
m_isInternalKey = true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the review.

  1. The code looks to be well organized and structured, with appropriate comments and function/variable names that make it easy to read and understand.
  2. It looks like const correctness has been applied in the right places, which should help prevent any bugs related to accidental modifications of values.
  3. The hsmLibPath, keyIndex, and password variables have been made const in the getter functions, and the setter functions have been modified to take const references, which will help prevent any unintended data modifications.
  4. The constructor has been modified to take a const reference instead of a string, which will help prevent any unwanted copies of the parameters being made.
  5. There are no obvious bugs or potential risks, but it might be worth checking for any memory leaks or unexpected behavior related to the variables declared in the class.

CRYPTO_LOG(INFO) << "[HsmSM2KeyPairFactory::HsmSM2KeyPairFactory]"
<< LOG_KV("_libPath", _libPath) << LOG_KV("lib_path", m_hsmLibPath);
}
HsmSM2KeyPairFactory(std::string _libPath) { m_hsmLibPath = _libPath; }
~HsmSM2KeyPairFactory() override {}

void setHsmLibPath(std::string _libPath) { m_hsmLibPath = _libPath; }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

......

The code in the patch looks good. There are no obvious bug risks, but there is an improvement suggestion. The constructor of the HsmSM2KeyPairFactory class was previously outputting the libPath and m_hsmLibPath to the log, but with this patch it is no longer doing so. It would be useful to have that logging information for debugging purposes, so I would suggest restoring the logging statement. Otherwise, the code looks good and should be safe to implement.

bcos::cppsdk::jsonrpc::JsonRpcServiceImpl::Ptr jsonRpcService() const
{
return m_jsonRpcService;
}
bcos::cppsdk::amop::AMOP::Ptr amop() const { return m_amop; }

bcos::cppsdk::event::EventSub::Ptr eventSub() const { return m_eventSub; }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review:

  1. The #include line for "JsonRpcServiceImpl.h" should be added inside the header file to ensure that it is properly included and initialized.

  2. The constructor should be updated to include the new parameter for the JsonRpcServiceImpl pointer.

  3. A public accessor method should be added for the jsonRpcService pointer.

  4. The start() and stop() methods should be updated to include initialization and deinitialization of the jsonRpcService.

  5. The destructor should also be updated to include the deinitialization of the jsonRpcService.

auto jsonRpcService = std::make_shared<JsonRpcServiceImpl>(_jsonRpc, transactionBuilder);
return jsonRpcService;
}

bcos::cppsdk::amop::AMOP::Ptr SdkFactory::buildAMOP(bcos::cppsdk::service::Service::Ptr _service)
{
auto amop = std::make_shared<bcos::cppsdk::amop::AMOP>();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a brief code review.

  1. The code patch added includes for JsonRpcInterface, JsonRpcServiceImpl, TransactionBuilder, TransactionBuilderService, and LogInitializer headers.
  2. The code patch also added a new method called buildJsonRpcService which uses a shared pointer to the JsonRpcImpl object, as well as a TransactionBuilder object, to create a new shared pointer to a JsonRpcServiceImpl object.
  3. In the buildSDK() method, the code patch added a parameter for the jsonRpcService shared pointer.
  4. Lastly, the code patch also added a call to LogInitializer::initLog() in the SdkFactory constructor.

In terms of potential bug risks, there is a potential risk that the new buildJsonRpcService() method may not work as expected, or that the parameters being passed are invalid. Additionally, there is a potential risk that the call to LogInitializer::initLog() may not be thread-safe.

In terms of improvement suggestions, it would be a good idea to add more comments to the code to explain what the new method is doing and why it is necessary. Additionally, it might be a good idea to add test cases to ensure that the new method is working as expected.

@@ -43,6 +44,8 @@ class SdkFactory : public std::enable_shared_from_this<SdkFactory>
std::shared_ptr<bcos::boostssl::ws::WsConfig> _config);
bcos::cppsdk::jsonrpc::JsonRpcImpl::Ptr buildJsonRpc(
bcos::cppsdk::service::Service::Ptr _service);
bcos::cppsdk::jsonrpc::JsonRpcServiceImpl::Ptr buildJsonRpcService(
bcos::cppsdk::jsonrpc::JsonRpcImpl::Ptr _jsonRpc);
bcos::cppsdk::amop::AMOP::Ptr buildAMOP(bcos::cppsdk::service::Service::Ptr _service);
bcos::cppsdk::event::EventSub::Ptr buildEventSub(bcos::cppsdk::service::Service::Ptr _service);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

First, I would like to check if there is any security risk in the code. Since this code patch includes adding a new line of code #include "rpc/JsonRpcServiceImpl.h", it is necessary to check if the header file and its content is safe. It should be checked to see if the header file contains any malicious code or any code that can cause a security risk.

Then, I would like to check if the code is properly formatted and easy to read. For example, the indentation of the code should be consistent, and the variable names should be meaningful. It will also be helpful to add comments to explain the logic behind the code so that it can be easily understood by other developers.

Finally, I would suggest to explore the possibility of refactoring the code so that it can be more efficient and maintainable. This can be done by optimizing the code, reducing redundant code, and making sure the code is DRY(Don't Repeat Yourself).

@@ -138,7 +138,6 @@ void AMOP::publish(
{
auto errorNew = BCOS_ERROR_PTR(wsMessage->status(),
std::string(wsMessage->payload()->begin(), wsMessage->payload()->end()));

AMOP_CLIENT(WARNING) << LOG_BADGE("publish") << LOG_DESC("publish response error")
<< LOG_KV("errorCode", errorNew->errorCode())
<< LOG_KV("errorMessage", errorNew->errorMessage());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review.

  1. The code patch is missing a line of code between lines 138 and 139. Without the line, the program may be prone to errors.

  2. The log message in line 141 should be changed to include more information such as the endpoint and parameters used to make the request. This will help with debugging if there are any issues.

  3. Line 142 should include the response status code in the log message. This will help with debugging and understanding the error.

@@ -81,7 +81,7 @@ void EventSub::doLoop()
m_timer->restart();
{
boost::shared_lock<boost::shared_mutex> lock(x_tasks);
EVENT_SUB(INFO) << LOG_BADGE("doLoop") << LOG_DESC("event sub tasks")
EVENT_SUB(INFO) << LOG_BADGE("doLoop") << LOG_DESC("event sub tasks report")
<< LOG_KV("working event sub count", m_workingTasks.size())
<< LOG_KV("suspend event sub count", m_suspendTasks.size());
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a brief code review

Code Review:
The patch above looks good, it is making a change to the EventSub::doLoop() function by changing the description of the event sub tasks from "event sub tasks" to "event sub tasks report" which more accurately reflects the purpose of the function. Additionally, it also adds logging information to report the size of working and suspended tasks to the log. No bugs were found in the patch and no improvements were suggested.

@@ -39,7 +39,7 @@ bool EventSubParams::verifyParams()
{
for (const auto& topic : m_topics[i])
{
if (!codec::abi::ContractEventTopic::validEventTopic(topic))
if (!codec::abi::ContractABIEventTopic::validEventTopic(topic))
{
return false;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a brief code review

This patch looks to be making changes to the EventSubParams class, replacing the ContractEventTopic class with the ContractABIEventTopic class. It also adds a verification step in the verifyParams() function to ensure that the event topics are valid.

Overall, the patch looks to be well written and should not have any major bugs. However, it would be good to add more tests to ensure that the new verification step is functioning as expected. Additionally, it might be beneficial to add comments to the code to explain why the changes were made.

@@ -135,7 +135,7 @@ class JsonRpcImpl : public JsonRpcInterface, public std::enable_shared_from_this
JsonRpcSendFunc sender() const { return m_sender; }
void setSender(JsonRpcSendFunc _sender) { m_sender = _sender; }

std::shared_ptr<bcos::boostssl::ws::WsService> service() const { return m_service; }
std::shared_ptr<bcos::cppsdk::service::Service> service() const { return m_service; }
void setService(std::shared_ptr<bcos::cppsdk::service::Service> _service)
{
m_service = _service;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the Code Review:

  1. The code patch looks like it is changing the type of the 'service' variable from boostssl::ws::WsService to cppsdk::service::Service. This is a good approach as it ensures that the data type is up-to-date and accurate.

  2. It appears that no other code changes were made, which is good. No new functionality or new bugs can be introduced.

  3. One improvement recommendation would be to provide a comment above the code patch explaining what is being changed and why. This would make it easier for future developers to understand what was done and why.

@@ -21,6 +21,7 @@
#include <bcos-cpp-sdk/rpc/Common.h>
#include <bcos-cpp-sdk/rpc/JsonRpcRequest.h>
#include <json/json.h>
#include <boost/exception/diagnostic_information.hpp>

using namespace bcos;
using namespace cppsdk;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review.

  1. The first thing to check is the coding style and indentation, it looks like it is properly indented and well structured.

  2. The added include statement for <boost/exception/diagnostic_information.hpp> seems to be appropriate here.

  3. We should make sure that the variables are declared properly and that it adheres to the right naming conventions.

  4. We should also make sure that the code is free of any syntax errors or warnings.

  5. We should also check if the code is compiled properly with no runtime errors.

  6. We should also confirm if the code meets the functional requirements, such as input validation, security checks, etc.

  7. We should also perform a unit test to ensure that the code works as expected.

Overall, the code looks good and there are no obvious errors. However, it is recommended to perform the above mentioned checks to ensure the code is bug-free and meets the functional requirements.


m_rpc->sendTransaction(_groupID, _nodeName, signedTransaction, false, std::move(_respFunc));
return transactionHash;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with code review:

  1. It's better to add more comments to explain the code, so that it can be more readable and understandable.
  2. Use const when declaring constant variables to improve code readability.
  3. You can use "auto" keyword instead of explicit type declaration, which can make code more concise.
  4. You can use "if constexpr" or "if constinit expression" to improve the readability of the code.
  5. The variable name should be more meaningful and easy to understand.
  6. You can use "nullptr" instead of "NULL" to improve the readability of the code.
  7. You can add log messages to the code for better debugging purpose.
  8. You can use smart pointer for memory safety.
  9. You can add some assertion in the code to detect potential bugs.


} // namespace jsonrpc
} // namespace cppsdk
} // namespace bcos No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review.

  1. Code readability - The code is well organized, it has sufficient indentation and good comments.
  2. Naming Convention - The variable names used in the code are descriptive and follow the standard naming conventions.
  3. Error handling - There is no error handling code present, it should be added to the code.
  4. Security - The code is not using any security measures like encryption or authentication, so there is a risk of data being compromised.
  5. Performance - There is no optimization code present, which can improve the performance of the code.


} // namespace jsonrpc
} // namespace cppsdk
} // namespace bcos No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the brief code review:

  1. The code looks well-structured and is easy to read.
  2. There is no obvious bugs in the code, but minor improvements can be made to make the code more readable and efficient.
  3. The function parameters of 'sendTransaction' can be better documented and more clearly defined.
  4. The class should be made final as it is not expected to be extended.
  5. A logging system can be added for debugging purpose.


} // namespace utilities
} // namespace cppsdk
} // namespace bcos No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review:

  1. There seems to be no bug risk in the code as all function calls and variables are properly used.
  2. The code can be improved by adding more comments to explain its purpose and how it works. Also, an error handling mechanism can be added if necessary.


} // namespace abi
} // namespace cppsdk
} // namespace bcos No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review.

  1. This code looks properly indented and organized.
  2. The code is following the style guide and uses descriptive variable names to make the code more readable.
  3. The code looks well commented and includes appropriate comments that explain the purpose of each section, making it easier to understand.
  4. It is using modern C++ features such as std::optional which improves the readability and maintainability of the code.
  5. Mutexes are used in the code to ensure thread safety.
  6. The code does not have any obvious bugs, however it is recommended to do further testing and debugging to ensure that there are no hidden bugs present.
  7. It would be beneficial to add some documentation to the code so that other developers can easily understand the purpose of the code and how it works.


} // namespace abi
} // namespace cppsdk
} // namespace bcos No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review.

Firstly, the code looks well organized with good indentation and comments. The naming of variables and functions are clear and easy to understand which is great.

Some minor improvements that could be done include adding a try-catch block around any potential exceptions and making sure the input parameters are valid before execution. It would also be wise to add more comments as needed throughout the code to make it easier for future readers.

Overall, the code looks well written and there don't appear to be any major risks or bugs.

}

private:
bcos::crypto::Hash::Ptr m_hashImpl;
codec::abi::ContractABICodec m_contractABICodec;

bcos::cppsdk::abi::ContractABITypeCodecSolImpl m_solCodec;
};

} // namespace abi
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a review.

The code patch seems to be making changes to the ContractABIEventTopic class. The changes seem to involve removing parts of the class such as the ContractABICodec and some of the methods for serializing data, such as u256ToTopic, i256ToTopic, and bytesNToTopic. Additionally, some methods such as hashImpl were removed and the class was renamed from ContractEventTopic to ContractABIEventTopic.

It is difficult to tell if the code is correct without knowing the context and purpose of the code. However, some possible issues may include:
-Removing methods that are needed by other parts of the program.
-Removing methods that are used in the processing of data.
-Renaming the class without considering the implications that this may have on other parts of the program.

Suggestions to improve the code may include:
-Documenting the changes that were made to the class.
-Adding unit tests to ensure that the code is working properly.
-Performing a code review with other members of the team to ensure that the code is correct.

m_type = strType;
m_baseType = baseType;
m_extents = extents;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:

  1. The patch should add some comments to explain what it does and why it is needed.

  2. There should be a check in reset() if the leftBracket is the last character in the string, otherwise it will throw an exception when running.

  3. It would be more secure to use std::stoul() instead of std::stoi() to convert string to size_t.

  4. In removeExtent(), it should return false if size is 0.

  5. The naming of variables could be clearer, it would be better to name the parameter in reset() as typeString.


} // namespace utilities
} // namespace cppsdk
} // namespace bcos No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review.

  1. The code is well indented.
  2. Comments have been provided to explain the purpose of the code.
  3. The code uses the correct naming conventions.
  4. The code uses the appropriate header files.
  5. The code is using the right data types.
  6. The code is using the right control flow structures.
  7. The code is using the right error handling techniques.
  8. There are no unnecessary variables or functions.
  9. The code does not contain any hardcoded values.
  10. The code is secure and does not contain any potential security issues.

};
} // namespace utilities
} // namespace cppsdk
} // namespace bcos No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review

  1. Your code looks good and well-structured.
  2. The indentation of your code should be consistent with 4 spaces instead of 2.
  3. The naming of variables should be more descriptive and meaningful to make it easier to understand the code.
  4. You should use proper comments to explain the logic of the code, especially for the complex logic.
  5. Make sure you include input validation for any user-provided input.
  6. Include proper logging and error handling for any potential errors.

{
BOOST_THROW_EXCEPTION(std::runtime_error("sign don't unsupport algorithm "));
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review:

  1. The code is well-structured and easy to read.
  2. All the variables and functions have meaningful names, which make it easier to understand the code.
  3. The code follows the coding style standard, which makes it easier to maintain.
  4. It is necessary to verify whether the parameters passed in are valid and add exception handling code.
  5. The logic of the code is clear, but it is necessary to consider whether there are dead cycles or infinite loops.
  6. Comments should be added to the code for better understanding.

};
} // namespace utilities
} // namespace cppsdk
} // namespace bcos No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a brief code review for Signature.h

  1. This code appears to be written in C++, and it looks like it is a class that provides methods for signing, verifying, recovering, and recovering the address from a hash.

  2. The code looks well-structured and organized, and it is easy to read and understand.

  3. There are no apparent bugs or security risks in this code patch.

  4. It might be beneficial to add more comments to explain the purpose and usage of each method in more detail. This will help future developers better understand the code.

  5. The parameter _hsmLibPath appears to be hardcoded in several places. It would be good to make this parameter configurable instead so that the library path can be changed easily.

#include <bcos-cpp-sdk/utilities/logger/LogInitializer.h>

std::once_flag bcos::cppsdk::LogInitializer::m_flag;
bcos::BoostLogInitializer* bcos::cppsdk::LogInitializer::m_logInitializer; No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a brief code review:

This patch adds an implementation of the LogInitializer class which is used to initialize a BoostLogInitializer. The code is properly commented and follows the coding guidelines of the project.

The code looks good, but there are a few points I would like to make. First, I would suggest adding a check to ensure that the LogInitializer class is only initialized once. This could be done using the std::once_flag variable that is already declared in the code. Second, it is also a good practice to add a destructor to the LogInitializer class so that the BoostLogInitializer can be properly cleaned up when the class is destroyed.

Overall, this code looks correct and should work without any issues.

static bcos::BoostLogInitializer* m_logInitializer;
};
} // namespace cppsdk
} // namespace bcos No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:

  1. The code looks well-structured and organized.
  2. The function initLog() is properly overloaded with two parameters to give users more choices.
  3. It's recommended to use a unique system-wide log file instead of different log files for each process.
  4. The function initLog() should be called only once, so it's better to use std::call_once to ensure this.
  5. The exception handling should be improved to avoid unexpected behaviors.
  6. It's also recommended to add comments to the code to make it easier to understand.

receiptData->readFrom(inputStream);
auto receiptDataJson = receiptData->writeToJsonString();
return receiptDataJson;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review:

  1. The code is missing some necessary comment for better understanding, for example, the parameters in the functions needs to explain what they mean.
  2. The security risk should be considered when using hash algorithm, like the algorithm collision and so on.
  3. Naming conventions should be followed. For example, class name should be start with capital letter.
  4. The exceptions and error handling should be added when necessary.
  5. Unit test should be added for each function.

std::make_unique<bcos::crypto::CryptoSuite>(std::make_shared<bcos::crypto::SM3>(),
std::make_shared<bcos::crypto::SM2Crypto>(), nullptr);
};
} // namespace bcos::cppsdk::utilities
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review:

The code looks good and is well structured. The use of comments and the descriptive variable names make it easy to understand the code. I would suggest using try/catch blocks to handle any exceptions that may arise while executing the code. Additionally, it would be beneficial to include a few test cases to check the functionality of the code. Overall, the code looks good and should be ready for production.

*/
virtual std::string decodeReceiptDataToJsonObj(const bcos::bytes& _receiptBytes) = 0;
};
} // namespace bcos::cppsdk::utilities No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!

I can see the patch added a class 'ReceiptBuilderInterface' which provides the functionality of creating TransactionData object from json file, calculating hash from TransactionData object, encoding receipt, and decoding receipt data to json object.

The code looks good in general, some minor points I would like to suggest as improvement:

  1. Use 'override' keyword in all the functions that are overridden.
  2. You can also add const qualifier in the parameter of function decodeReceiptDataToJsonObj() as it doesn't modify the parameter.
  3. Add a const qualifier in the return type of function calculateReceiptDataHash() as the function doesn't modify any internal state of class.
  4. Use a consistent coding style for indentation.

Hope this helps.

};
} // namespace utilities
} // namespace cppsdk
} // namespace bcos No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review for the above code patch:

  1. The code looks well structured and organized.
  2. It is using a shared pointer for the TransactionBuilderInterface, which is a good practice for memory management.
  3. There should be some checks to validate the parameters passed to the function createSignedTransaction.
  4. A better naming convention could be used for variables, like using camelCase instead of under_score.
  5. It is missing comments for the class and functions, which can help other developers to understand it better.
  6. There should be some unit tests added to test the functionality of the code.

{
return parseError(getSystemCode());
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review.

First of all, the code looks well organized and clean, with good indentation and comments. There are no obvious errors or typos in the code, which makes it easier to read.

The code is written in C++ and there are a few things that could be improved. Firstly, it would be better to use more descriptive variable names instead of generic ones like 'buffer' and 'err'. Also, it would be helpful to add comments to explain the purpose of each function, especially for more complex functions. Finally, it would be good to add error handling to the code to make sure that it is robust and can handle any issues that may arise.

#else
#define SVT_DLL_API
#endif

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the code review:

  1. The code uses a number of preprocessor directives to define the target platform and determine what libraries to include. This is efficient and helps ensure that the code is portable across different platforms.

  2. The code also uses preprocessor directives to define the UTIL and SVT DLL import/export macros. This helps ensure that the code is only exported/imported when necessary.

  3. The code does not appear to have any bugs at first glance.

  4. As a potential improvement, it may be beneficial to add a comment before each preprocessor directive indicating what the directive does, as this would make the code more readable and understandable.


}

#endif
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The variable names are not descriptive and need to be made more meaningful.
  2. Check for any potential memory leaks.
  3. Check for any potential race conditions.
  4. Check if any of the functions used are deprecated.
  5. Check for any unused code or variables.
  6. Check if any of the function calls can be optimized.

#endif

#endif

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a brief code review

  1. Firstly, you have used the header guards to avoid double inclusion of header files. This is good coding practice.

  2. The code you have written checks if the target platform is Windows and if yes, then it declares the strptime function. This is also good coding practice.

  3. You have used the correct indentation for the code. This makes the code easier to read and understand.

Overall, the patch looks good and there are no issues that need to be addressed.

@@ -21,6 +21,7 @@
#include <bcos-cpp-sdk/ws/BlockNumberInfo.h>
#include <bcos-cpp-sdk/ws/Common.h>
#include <json/json.h>
#include <boost/exception/diagnostic_information.hpp>

using namespace bcos;
using namespace bcos::cppsdk;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the code review:

  1. The patch includes an additional header file 'boost/exception/diagnostic_information.hpp', which is used to provide more detailed reporting of exceptions. This should help with debugging any errors that may arise.

  2. The code looks correct and no major bug risk is seen.

  3. Code comments can be added to make it easier to read and understand.

  4. There are no major improvements suggested at this time, but it is recommended that you consider making the code more efficient, readable, and maintainable.

endif()
target_link_libraries(broadcast PUBLIC ${BCOS_CPP_SDK_TARGET} bcos-boostssl bcos-utilities jsoncpp_static OpenSSL::SSL OpenSSL::Crypto)

add_executable(subscribe subscribe.cpp)
if (NOT WIN32)
target_compile_options(subscribe PRIVATE)
target_compile_options(subscribe PRIVATE -Wno-error -Wno-unused-variable)
endif()
target_link_libraries(subscribe PUBLIC ${BCOS_CPP_SDK_TARGET} bcos-boostssl bcos-utilities jsoncpp_static OpenSSL::SSL OpenSSL::Crypto) No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review

The code patch looks good overall. The line "-Wno-error -Wno-unused-variable" has been added to the target_compile_options for the publish, broadcast and subscribe executables, which will tell the compiler to ignore certain types of errors and warnings. This should help reduce the number of warnings generated when compiling the code.

However, as a recommendation, it would be better to only add specific error/warning suppression flags for each particular warning or error that is being generated, instead of adding "-Wno-error -Wno-unused-variable" for all executables. This will help ensure that all potential issues are addressed properly and that the code remains clean and maintainable.

@@ -5,6 +5,6 @@ file(GLOB HEADERS "*.h")

add_executable(eventsub eventsub.cpp)
if (NOT WIN32)
target_compile_options(eventsub PRIVATE -Wno-unused-variable)
target_compile_options(eventsub PRIVATE -Wno-error -Wno-unused-variable)
endif()
target_link_libraries(eventsub PUBLIC ${BCOS_CPP_SDK_TARGET} bcos-boostssl bcos-utilities jsoncpp_static OpenSSL::SSL OpenSSL::Crypto) No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review

This code patch looks good. The author has added the option -Wno-error to the target_compile_options to suppress an unused variable warning. This is a good practice for suppressing warnings and should help prevent any potential bugs in the future. I would also suggest that the author add some comments to explain why this option was added. Furthermore, it might be beneficial to consider adding other compile time options (e.g. -Wall) that could help detect potential bugs.

@@ -5,6 +5,6 @@ file(GLOB HEADERS "*.h")

add_executable(blocknotifier blocknotifier.cpp)
if (NOT WIN32)
target_compile_options(blocknotifier PRIVATE -Wno-unused-variable)
target_compile_options(blocknotifier PRIVATE -Wno-error -Wno-unused-variable)
endif()
target_link_libraries(blocknotifier PUBLIC ${BCOS_CPP_SDK_TARGET} bcos-boostssl bcos-utilities jsoncpp_static OpenSSL::SSL OpenSSL::Crypto) No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review

A code review is an important step in the development process, as it helps to identify any potential bugs or issues with the code. It also provides an opportunity to suggest improvements or optimizations that may not have been considered. In this case, the code patch adds a new compiler option -Wno-error to the target_compile_options function call. This new option tells the compiler to ignore any errors that may occur, which may help to reduce the number of bugs in the code. Additionally, the existing -Wno-unused-variable option is kept in place, which will help to prevent any unused variables from being included in the compiled code. Overall, this code patch appears to be beneficial and should help to improve the code's quality.

if (NOT WIN32)
target_compile_options(deploy_hello PRIVATE -Wno-error -Wno-unused-variable)
endif()
target_link_libraries(deploy_hello PUBLIC ${BCOS_CPP_SDK_TARGET} bcos-crypto bcos-boostssl bcos-utilities jsoncpp_static OpenSSL::SSL OpenSSL::Crypto)

add_executable(tx_sign_perf tx_sign_perf.cpp)
target_link_libraries(tx_sign_perf PUBLIC ${BCOS_CPP_SDK_TARGET} ${TARS_PROTOCOL_TARGET})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review

  1. The code looks good overall, however there are some improvements that can be made.
  2. The target_compile_options should include the -Wno-error flag to suppress compiler warnings.
  3. The target_link_libraries should include the ${TARS_PROTOCOL_TARGET} library for the tx_sign_perf executable.
  4. It is also recommended to add the -Wall and -Wextra flags to the target_compile_options to enable more warnings from the compiler.
  5. Additionally, the code should be tested thoroughly to ensure it works correctly and bug-free.

// auto u256Value = transactionBuilder->genRandomUint256();
// (void)u256Value;
auto u256Value = transactionBuilder->genRandomUint256();
(void)u256Value;
}

auto endPoint = std::chrono::high_resolution_clock::now();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the code review:

  1. The code seems to be using some libraries for functions such as TransactionBuilder, FixedBytes, etc. It would be better to include the library files at the top of the code for better readability.
  2. There is a lack of comments in the code which makes it difficult to understand the purpose of the code.
  3. The variable names used are not meaningful and can be confusing.
  4. Unused variables should be removed from the code to avoid confusion.
  5. The code should include proper error handling.
  6. The code should be tested thoroughly before deploying it.

"group", 1112, std::shared_ptr<bcos::crypto::KeyPairInterface>(std::move(keyPair)));
auto txPair = transactionBuilder->createSignedTransaction(
*keyPair, group_id, chain_id, "", code, "", block_limit, 0);
txHash = txPair.first;
}

auto endPoint = std::chrono::high_resolution_clock::now();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the code review:

  1. The code has included the required header files and libraries for the program.
  2. The code has used explicit types to define variables, which is good practice.
  3. The code has used standard library functions and classes, which are preferred over custom implementations.
  4. The code has used the correct data types for variables and constants.
  5. The code has used proper indentation and spacing, making it easier to read and understand.
  6. There is a small bug risk due to the use of raw pointers instead of smart pointers. It is recommended to use smart pointers to avoid potential memory leaks.
  7. The code could be improved by using more descriptive variable names, as well as by adding additional comments to explain the logic.

bxq2011hust
bxq2011hust previously approved these changes Mar 10, 2023
wenlinlee
wenlinlee previously approved these changes Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants