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

Updated Transaction::serialized and Signature::fromHex #13

Merged
merged 3 commits into from
Apr 26, 2024

Conversation

DanHouseman
Copy link
Contributor

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::serialized Method:
For Transaction::serialized, the main areas where errors could occur include:

  1. Invalid or unexpected data types or values.
  2. Issues in the utility functions like utils::intToBytes or utils::fromHexString.

Code Efficiency:

  1. The use of a lambda function (appendSerializedData) reduces redundancy and improves readability by encapsulating repetitive operations.
  2. The repeated clearing of temp_val could be handled better by using local RLPValue object within the scope of their use. This reduces the need for manual clearing and potential mistakes.

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.
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the data parameter here shadows the Transaction.data member. -Werror is set so this is causing the tests to fail atm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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) {}

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@darcys22
Copy link
Collaborator

This is amazing, thank you for the PR! Happy to merge this once it passes CI

DanHouseman and others added 2 commits April 18, 2024 07:07
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 darcys22 merged commit 363c7ff into oxen-io:master Apr 26, 2024
1 check passed
@DanHouseman
Copy link
Contributor Author

@darcys22 awesome! Thanks! I look forward to getting into some more code!

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.

2 participants