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

Moves precompile-utils from moonbeam into frontier. #1150

Merged
merged 15 commits into from Aug 29, 2023

Conversation

rimbi
Copy link
Contributor

@rimbi rimbi commented Aug 11, 2023

What does it do?

Moves precompile-utils from moonbeam into frontier.

  • Moves the following crates to frontier repo:
    • precompile-utils
    • precompile-utils-macro
    • precompile-utils-tests-external

@rimbi rimbi requested a review from sorpaas as a code owner August 11, 2023 10:58
@rimbi
Copy link
Contributor Author

rimbi commented Aug 11, 2023

Cargo.toml Outdated
Comment on lines 34 to 36
"utils/precompiles",
"utils/precompiles/macro",
"utils/precompiles/tests-external",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"utils/precompiles",
"utils/precompiles/macro",
"utils/precompiles/tests-external",
"precompiles",
"precompiles/macro",
"precompiles/tests-external",

This utils path seems unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated
rlp = { version = "0.5.2", default-features = false }
scale-codec = { package = "parity-scale-codec", version = "3.6.4", default-features = false, features = ["derive"] }
scale-info = { version = "2.9.0", default-features = false, features = ["derive"] }
serde = { version = "1.0", default-features = false, features = ["derive", "alloc"] }
serde_json = "1.0"
sha3 = { version = "0.10", default-features = false }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sha3 = { version = "0.10", default-features = false }

we could use sp_core::hashing directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Moonbeam. If not, see <http://www.gnu.org/licenses/>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to update these licence headers? Keep it the same as other files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks! Done!

Copy link
Member

Choose a reason for hiding this comment

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

Unlike individual contributors, I don't think "PureStake Inc" signed any CLA with Parity, so we can't just remove the previous license headers. The previous attribution should be kept, so the end result looks something like this:

// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0
// This file is part of Frontier.
//
// Copyright (c) 2019-2022 PureStake Inc.
// Copyright (c) 2023 Parity Technologies (UK) Ltd.
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with Wei's result but I'll get confirmation from the Moonbeam Foundation (PureStake Inc. will cease to exist at some point)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confirm it is fine

Copy link
Member

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

LGTM, but the code probably better belongs in frame/evm/precompile.

@koushiro
Copy link
Collaborator

koushiro commented Aug 29, 2023

but the code probably better belongs in frame/evm/precompile.

If we move the code to frame/evm/precomile, I think the directory tree will may be too deep.

And the introduced precompile macro in this PR is more used for customization, while the precompiles in frame/evm/precomile dir are the standard of ethereum ? 🤔️

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file seems to be your local development configuration? @rimbi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, thanks! Removed.

@sorpaas
Copy link
Member

sorpaas commented Aug 29, 2023

If we move the code to frame/evm/precomile, I think the directory tree will may be too deep.

And the introduced precompile macro in this PR is more used for customization, while the precompiles in frame/evm/precomile dir are the standard of ethereum ? 🤔️

It's not a big deal anyway. We can also move frame/evm/precompile out in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants