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

fp-account #1019

Merged
merged 29 commits into from
Mar 17, 2023
Merged

fp-account #1019

merged 29 commits into from
Mar 17, 2023

Conversation

bernardoaraujor
Copy link
Contributor

@bernardoaraujor bernardoaraujor commented Mar 15, 2023

Based on Moonbeam's AccountId20 implementation:

Introduces a new fp-account crate into primitives, as an alternative to Frontier's H256->H160 truncation mapping.

H160 addresses live natively on the Substrate runtime, and there's no need for translation between H256 and H160.

@bernardoaraujor bernardoaraujor added the enhancement New feature or request label Mar 15, 2023
frame/evm/src/lib.rs Outdated Show resolved Hide resolved
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 otherwise

… directly

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
bernardoaraujor and others added 3 commits March 15, 2023 23:53
Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
frame/ethereum/src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
bernardoaraujor and others added 11 commits March 16, 2023 00:15
Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
}

impl From<ecdsa::Public> for AccountId20 {
fn from(pk: ecdsa::Public) -> Self {
Copy link
Contributor

@yjhmelody yjhmelody Mar 16, 2023

Choose a reason for hiding this comment

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

I think this logic should not be implmented in a From. It is not a cheap convert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what would you suggest as alternative?

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be okay. I don't recall any restrictions that we shouldn't use From for expensive convert.


#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Default)]
#[derive(Encode, Decode, MaxEncodedLen, TypeInfo)]
pub struct AccountId20(pub [u8; 20]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add more docs for these types. e.g. eth compatible address blah blah.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sorpaas sorpaas merged commit 43616e4 into master Mar 17, 2023
@sorpaas sorpaas deleted the bar/fp-account branch March 17, 2023 21:28
Dinonard pushed a commit to AstarNetwork/frontier that referenced this pull request Mar 22, 2023
* bootstrap fp-account

* clippy

* make IdentityAddressMapping support From<H160>

* clippy

* fix import warning

* fix pallet-evm tests

* fix pallet-hotfix-sufficients tests

* use libsecp256k1::PublicKey::parse_compressed and sp_core::keccak_256 directly

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* simplify keccak_256 hashing

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* format derives

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* merge_derives = false

* fix imports

* impl Debug for AccountId20

* simplify Display for EthereumSigner

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* fmt

* simplify keccak_256 hashing

* simplify ecdsa path

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* format derives

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* use sp_io for keccak_256

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* remove sha3 dependency

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* remove sha3 dependency

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* remove sha3 dependency

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* remove sha3 import

* remove sha3 dependency

* simplify keccak_256 hashing on tests

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* remove serde imports

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>

* add account docs

* remove sp-keyring from template deps

---------

Co-authored-by: Qinxuan Chen <koushiro.cqx@gmail.com>
Co-authored-by: Wei Tang <wei@pacna.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants