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.

Sign up for GitHub

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

Jump to bottom

feat: update & refactor precompiles #1100

Merged
merged 23 commits into from
Dec 18, 2023
Merged

feat: update & refactor precompiles #1100

merged 23 commits into from
Dec 18, 2023

Conversation

ashutoshvarma
Copy link
Member

@ashutoshvarma ashutoshvarma commented Dec 11, 2023 •
edited
Loading

Pull Request Summary
Update precompiles to use new precompiles utils

Precompiles to Update

  • assets-erc20
  • dppstaking
  • ecdsa
  • sr25519
  • xvm
  • xcm/xtokens
  • unified-accounts (merge upstream first)

Runtimes to update

  • Astar
  • Shiden
  • Shibuya
  • Local

TODOs

  • test all the precompiles args
  • test the filters (especially for dispatch) and remove blacklisted hack

@ashutoshvarma ashutoshvarma changed the title featL update & refactor precompiles feat: update & refactor precompiles Dec 11, 2023
@ashutoshvarma ashutoshvarma added shiden related to shiden runtime astar Related to Astar shibuya related to shibuya runtime This PR/Issue is related to the topic “runtime”. labels Dec 11, 2023
Dinonard
Dinonard reviewed Dec 11, 2023
View reviewed changes
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

This is partial review, I've stopped at XCM. Will continue tomorrow.

precompiles/assets-erc20/src/lib.rs Show resolved Hide resolved
precompiles/assets-erc20/src/mock.rs Outdated Show resolved Hide resolved
precompiles/dapps-staking/src/lib.rs Outdated Show resolved Hide resolved
precompiles/sr25519/src/lib.rs
public: H256,
signature: BoundedBytes<SR25519SignatureBytes>,
message: UnboundedBytes,
) -> EvmResult<bool> {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any cost associated to this call?

It's not a question related to your PR, but a general one.

Copy link
Member Author

Choose a reason for hiding this comment

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

There would be, a minor one for the signature verification.
It needs to be benchmarked like many other precompiles method that we have TODOs for

Copy link
Member

Choose a reason for hiding this comment

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

I meant the cost that we charge the caller, but you answered my question regardless 🙂

We should make an item to address this.

precompiles/utils/Cargo.toml Outdated Show resolved Hide resolved
Dinonard
Dinonard reviewed Dec 12, 2023
View reviewed changes
precompiles/xcm/src/lib.rs Outdated Show resolved Hide resolved
runtime/astar/src/precompiles.rs Outdated Show resolved Hide resolved
runtime/astar/src/precompiles.rs Outdated Show resolved Hide resolved
Dinonard
Dinonard reviewed Dec 13, 2023
View reviewed changes
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

Checked the delta only, single comment regarding the ranges.

runtime/astar/src/precompiles.rs Show resolved Hide resolved
@ashutoshvarma ashutoshvarma marked this pull request as ready for review December 14, 2023 06:36
Dinonard
Dinonard previously approved these changes Dec 15, 2023
View reviewed changes
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

LGTM

PierreOssun
PierreOssun reviewed Dec 15, 2023
View reviewed changes
precompiles/xcm/src/lib.rs Show resolved Hide resolved
precompiles/xvm/src/lib.rs Show resolved Hide resolved
runtime/shiden/src/precompiles.rs Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

Code Coverage

Package Line Rate Branch Rate Health
pallets/xc-asset-config/src 53% 0% ➖
precompiles/xcm/src 72% 0% ➖
chain-extensions/types/dapps-staking/src 0% 0% ❌
precompiles/assets-erc20/src 81% 0% ✔
primitives/src/xcm 66% 0% ➖
chain-extensions/unified-accounts/src 0% 0% ❌
chain-extensions/dapps-staking/src 0% 0% ❌
chain-extensions/pallet-assets/src 0% 0% ❌
chain-extensions/types/unified-accounts/src 0% 0% ❌
chain-extensions/types/xvm/src 0% 0% ❌
pallets/unified-accounts/src 84% 0% ✔
precompiles/dapps-staking/src 94% 0% ✔
precompiles/sr25519/src 64% 0% ➖
chain-extensions/xvm/src 0% 0% ❌
chain-extensions/types/assets/src 0% 0% ❌
pallets/dapps-staking/src 81% 0% ✔
pallets/dynamic-evm-base-fee/src 81% 0% ✔
pallets/block-rewards-hybrid/src 87% 0% ✔
pallets/contracts-migration/src 0% 0% ❌
pallets/ethereum-checked/src 48% 0% ❌
precompiles/unified-accounts/src 100% 0% ✔
precompiles/substrate-ecdsa/src 74% 0% ➖
precompiles/xvm/src 74% 0% ➖
pallets/xvm/src 40% 0% ❌
pallets/collator-selection/src 69% 0% ➖
primitives/src 65% 0% ➖
pallets/dapps-staking/src/pallet 85% 0% ✔
Summary 56% (1615 / 2886) 0% (0 / 0) ➖

Minimum allowed line rate is 50%

PierreOssun
PierreOssun approved these changes Dec 18, 2023
View reviewed changes
Copy link
Member

@PierreOssun PierreOssun left a comment

Choose a reason for hiding this comment

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

LGTM

@Dinonard Dinonard merged commit ae668a0 into master Dec 18, 2023
8 checks passed
@Dinonard Dinonard deleted the feat/precompiles-refactor branch December 18, 2023 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PierreOssun PierreOssun PierreOssun approved these changes

@Dinonard Dinonard Dinonard approved these changes

Assignees
No one assigned
Labels
astar Related to Astar runtime This PR/Issue is related to the topic “runtime”. shibuya related to shibuya shiden related to shiden runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ashutoshvarma @Dinonard @PierreOssun