Updated Transaction::serialized and Signature::fromHex #13
Conversation
Refine Transaction::serialized and Signature::fromHex methods with focus on implementing error handling, optimizing 'reuse vs object creation' inefficiency and enhancing documentation. Improves efficiency and maintainability of the code.
src/transaction.cpp
Outdated
| access_list.setArray(); | ||
| arr.push_back(access_list); | ||
| // Helper lambda for repetitive operations to convert and append data | ||
| auto appendSerializedData = [&](const auto& data, auto conversionFunction) { |
There was a problem hiding this comment.
the data parameter here shadows the Transaction.data member. -Werror is set so this is causing the tests to fail atm
There was a problem hiding this comment.
Sorry about that. I don't know why I didn't catch that.
I pushed a fix for that up to this branch and made a few more optimizations.
(compiler hint to inline the helper function, pre-allocation of memory, and some efficient conditional logic.)
Thinking about it now... Have you considered explicitly setting all transaction-related defaults in the constructor? This would ensure every transaction starts with a known good state.
Transaction(const std::string& _to, uint64_t _value, uint64_t _gasLimit = 21000, const std::string& _data = "", uint64_t _chainId = 1, uint64_t _nonce = 0, uint64_t _maxPriorityFeePerGas = 1, uint64_t _maxFeePerGas = 1)
: chainId(_chainId), nonce(_nonce), maxPriorityFeePerGas(_maxPriorityFeePerGas), maxFeePerGas(_maxFeePerGas), to(_to), value(_value), gasLimit(_gasLimit), data(_data) {}
There was a problem hiding this comment.
The goal with the constructor was just to require the bare minimum needed to get a transaction going. I was assuming that most of the other items were going to be set by the providers defaults (ie id say its pretty rare that the user wants to manually set chainID but instead want to rely on the node to provide it), so putting them into a known bad state means that we can check if it has changed before we query the node.
I do like that the constructor at the moment is minimal, but it does assume that the default pathway will be for it to go through the signer/provider and things will be changed.
Transaction tx("0x2Ccb8b65024E4aA9615a8E704DFb11BE76674f1F", 100000000000000);
const auto hash = signer.sendTransaction(tx, seckey);
However I do see the point, tx above does not immediately come into a ready state. It would need extra steps like this
tx.chainId = 1;
tx.nonce = 1;
If you for example wanted the tx to be exported immediately without the signer/provider cleaning things up.
|
This is amazing, thank you for the PR! Happy to merge this once it passes CI |
appendDataToRLP is declared inline to hint to the compiler that it should avoid function call overhead by integrating the function's body at each call site. Using arr.reserve() to pre-allocate memory for the RLPValue array based on whether a signature is present,. This minimizes dynamic memory allocation overhead. Adjusting the pre-allocation when signatures are present to handle additional elements, ensuring efficient memory use without redundant resizing. This is very efficient conditional logic. Using this->data for EXPLICIT MEMBER ACCESS. This (no pun intended) clearly indicates the use of the member variable, avoiding any ambiguity with local variables or parameters
|
@darcys22 awesome! Thanks! I look forward to getting into some more code! |
Refine Transaction::serialized and Signature::fromHex methods drilling in on adding error handling, optimizing 'reuse vs object creation' inefficiency and enhancing documentation. This improves efficiency and maintainability of the code. (Yes I'm being pedantic but vroom vroom!)
Error Handling for
Transaction::serializedMethod:For Transaction::serialized, the main areas where errors could occur include:
Code Efficiency:
appendSerializedData) reduces redundancy and improves readability by encapsulating repetitive operations.temp_valcould be handled better by using localRLPValueobject within the scope of their use. This reduces the need for manual clearing and potential mistakes.