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

Apply Substrate Review comments #146

Closed
tonimateos opened this issue Nov 8, 2023 · 2 comments · Fixed by #166
Closed

Apply Substrate Review comments #146

tonimateos opened this issue Nov 8, 2023 · 2 comments · Fixed by #166
Labels
High Priority Proposed to focus on this when other tasks are present Sprint Candidate Proposed by PO for the very next sprint

Comments

@tonimateos
Copy link
Contributor

tonimateos commented Nov 8, 2023

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

Not relevant anymore:

   - [DONE] laos-evolution-node:
    - pallet-living-assets-evolution:
      Rename `TemplateModule` -> `LivingAssetsModule` in `[mock.rs](http://mock.rs/)` and `[tests.rs](http://tests.rs/)`

    - [DONE] Please benchmark the `pallet-living-assets-ownership`

    - [DONE as part of Unified Accounts]AccountIdToH160 and H160ToAccountId 
        WeI'd recommend to evaluate another approach such as:
            - "binding H160 to SS58 AccountId" via an extrinsic that verifies an Ethereum signature to prove that an H160 address is owned by the SS58 owner.
            That way the H160 owner would be able to sign and send transactions directly to the EVM (e.g. via MetaMask, Hardhat, etc).

laos-bridge:
- Code looks good but as it is a bridge (with its big complexity) we'd recommend to audit (or part of) it later (no need to do it in M1).

@tonimateos tonimateos added Grooming Needed Extra attention is needed Sprint Candidate Proposed by PO for the very next sprint High Priority Proposed to focus on this when other tasks are present and removed Grooming Needed Extra attention is needed labels Nov 8, 2023
@ghost ghost self-assigned this Nov 9, 2023
@ghost ghost linked a pull request Nov 10, 2023 that will close this issue
@ghost ghost removed their assignment Nov 14, 2023
@asiniscalchi asiniscalchi self-assigned this Nov 14, 2023
@asiniscalchi
Copy link
Member

asiniscalchi commented Nov 14, 2023

eth block gas limit

Image

@asiniscalchi
Copy link
Member

asiniscalchi commented Nov 14, 2023

txPool ethereum

Image

@asiniscalchi asiniscalchi removed their assignment Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority Proposed to focus on this when other tasks are present Sprint Candidate Proposed by PO for the very next sprint
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants