Apply SBP review comments

[ghost]

As a LAOS dev, I want to make sure that the review provided by the SBP is duly taken care of, so my code is better

ACCEPTANCE:

we apply the changes we deem necessary from the provided review
The review test:

Still relevant:

laos-ownership-node:
      - Runtime is using 60M as BlockGasLimit, it should be 15M.


It seems that `MAXIMUM_BLOCK_WEIGHT` is miscalculated:
          ```
          pub const MAXIMUM_BLOCK_WEIGHT: Weight =
            Weight::from_parts(WEIGHT_REF_TIME_PER_SECOND.saturating_mul(2), 5 * 1024 * 1024);
          -->
          pub const MAXIMUM_BLOCK_WEIGHT: Weight =
            Weight::from_parts(WEIGHT_REF_TIME_PER_SECOND.saturating_div(2), 5 * 1024 * 1024);
          ```

We'd recommend to add the "txpool" RPC methods to your collators:
            https://github.com/paritytech/frontier/blob/master/template/node/src/rpc/eth.rs#L193

[ghost]

/review

[github-actions]

PR Analysis

• 🎯 Main theme: The PR seems to be focused on applying comments from a previous review, specifically related to the Ethereum compatibility layer and block weight computation in the Ownership chain.
• 📝 PR summary: This PR applies changes to the Ethereum compatibility layer in the Ownership chain, including the addition of a transaction pool API server and changes to the filter pool. It also modifies the computation of the maximum block weight in the Ownership chain primitives, and adds a new feature flag for the transaction pool in the node's Cargo.toml.
• 📌 Type of PR: Refactoring
• 🧪 Relevant tests added: No
• ⏱️ Estimated effort to review [1-5]: 3, due to the changes in multiple files and the need to understand the Ethereum compatibility layer and block weight computation.
• 🔒 Security concerns: No security concerns found

PR Feedback

• 💡 General suggestions: The changes seem to be well thought out and are likely to improve the Ethereum compatibility of the Ownership chain. However, it would be beneficial to include more context in the PR description to help reviewers understand the motivation behind these changes. Additionally, it would be helpful to include tests to ensure that these changes work as expected.

• 🤖 Code feedback:

  • relevant file: ownership-chain/node/src/rpc/eth.rs
    suggestion: Consider handling potential errors when merging the transaction pool API server into the I/O handler. [important]
    relevant line: io.merge(fc_rpc::TxPool::new(client, graph).into_rpc())?;

  • relevant file: ownership-chain/primitives/src/lib.rs
    suggestion: Ensure that the change in block weight computation does not negatively impact the performance of the Ownership chain. [medium]
    relevant line: Weight::from_parts(WEIGHT_REF_TIME_PER_SECOND.saturating_div(2), 5 * 1024 * 1024);

  • relevant file: ownership-chain/node/Cargo.toml
    suggestion: Make sure that the new feature flag for the transaction pool is properly documented and that its impact on the build process is well understood. [medium]
    relevant line: txpool = ["fc-rpc/txpool"]

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

[ghost]

BlockGasLimit computation depends on this constant, so by changing this we also address the comment about gas limit (reducing it from 60M to 15M).

[ghost]

pub BlockGasLimit: U256 = U256::from(NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT.ref_time() / WEIGHT_PER_GAS);

[asiniscalchi]

/improve better name for the test

[asiniscalchi]

/ask what about a better name for the test ?

[github-actions]

Suggestion: Consider adding a description for the "txpool" feature for better understanding of its purpose.

Suggested change

	[features]
	default = []
	default = [
	"txpool"
	]
	txpool = ["fc-rpc/txpool"]
	[features]
	# Enables transaction pool functionality
	default = [
	"txpool"
	]
	txpool = ["fc-rpc/txpool"]

[github-actions]

Question: what about a better name for the test ?

Answer:
The test name test_constants is quite generic and does not provide much information about what the test is doing. It would be better to name tests in a way that describes their purpose or the specific functionality they are testing.

For instance, this test could be renamed to test_block_and_gas_limit_constants as it is specifically testing the block weights and gas limit constants. This would make it easier for other developers to understand what the test is for without having to read through the test code.

Remember, a good test name should quickly and accurately describe what the test does. It can be a bit longer if necessary - clarity is the key.

[asiniscalchi]

test to check txpool is working missing