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

Add 20-byte account id to subxt_core #1638

Merged
merged 3 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion codegen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ fn default_derives(crate_path: &syn::Path) -> DerivesRegistry {
fn default_substitutes(crate_path: &syn::Path) -> TypeSubstitutes {
let mut type_substitutes = TypeSubstitutes::new();

let defaults: [(syn::Path, syn::Path); 12] = [
let defaults: [(syn::Path, syn::Path); 13] = [
(
parse_quote!(bitvec::order::Lsb0),
parse_quote!(#crate_path::utils::bits::Lsb0),
Expand All @@ -371,6 +371,10 @@ fn default_substitutes(crate_path: &syn::Path) -> TypeSubstitutes {
parse_quote!(sp_core::crypto::AccountId32),
parse_quote!(#crate_path::utils::AccountId32),
),
(
parse_quote!(fp_account::AccountId20),
Copy link
Member

@niklasad1 niklasad1 Jun 13, 2024

Choose a reason for hiding this comment

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

Maybe add a comment that the type comes from frontier which probably works for frontier/moonbeam but probably not for all ethereum-based chains.

I have no strong opinions whether to support this override or not, doesn't hurt I guess...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mmm it might be that we can add a few overrides if needed to cater for the common chains, but otherwise the user can do this themselves.

I was hesitent but don't see a reason not to add this since its super unlikely to ever break anything that doesn't care :)

parse_quote!(#crate_path::utils::AccountId20),
),
(
parse_quote!(sp_runtime::multiaddress::MultiAddress),
parse_quote!(#crate_path::utils::MultiAddress),
Expand Down
3 changes: 3 additions & 0 deletions core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ sp-core = { workspace = true, optional = true }
sp-runtime = { workspace = true, optional = true }
tracing = { workspace = true, default-features = false }

# AccountId20
keccak-hash = { workspace = true}
niklasad1 marked this conversation as resolved.
Show resolved Hide resolved

[dev-dependencies]
assert_matches = { workspace = true }
bitvec = { workspace = true }
Expand Down
163 changes: 163 additions & 0 deletions core/src/utils/account_id20.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
// Copyright 2019-2024 Parity Technologies (UK) Ltd.
// This file is dual-licensed as Apache-2.0 or GPL-3.0.
// see LICENSE for license details.

//! `AccountId20` is a repressentation of Ethereum address derived from hashing the public key.

use core::fmt::Display;

use alloc::format;
use alloc::string::String;
use codec::{Decode, Encode};
use keccak_hash::keccak;
use serde::{Deserialize, Serialize};

#[derive(
Copy,
Clone,
Eq,
PartialEq,
Ord,
PartialOrd,
Encode,
Decode,
Debug,
scale_encode::EncodeAsType,
scale_decode::DecodeAsType,
scale_info::TypeInfo,
)]
/// Ethereum-compatible `AccountId`.
pub struct AccountId20(pub [u8; 20]);

impl AsRef<[u8]> for AccountId20 {
fn as_ref(&self) -> &[u8] {
&self.0[..]
}
}

impl AsRef<[u8; 20]> for AccountId20 {
fn as_ref(&self) -> &[u8; 20] {
&self.0
}
}

impl From<[u8; 20]> for AccountId20 {
fn from(x: [u8; 20]) -> Self {
AccountId20(x)
}
}

impl AccountId20 {
/// Convert to a public key hash
pub fn checksum(&self) -> String {
let hex_address = hex::encode(self.0);
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 code is just moved but we think we could get rid of this hex::encode call it should be possible to iterate over the bytes instead of allocating a String.

This probably doesn't matter in practice but just a thought :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it really that important?

Copy link
Member

Choose a reason for hiding this comment

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

no, I don't think it matters just an unnecessary allocation :)

let hash = keccak(hex_address.as_bytes());

let mut checksum_address = String::with_capacity(42);
checksum_address.push_str("0x");

for (i, ch) in hex_address.chars().enumerate() {
// Get the corresponding nibble from the hash
let nibble = hash[i / 2] >> (if i % 2 == 0 { 4 } else { 0 }) & 0xf;

if nibble >= 8 {
checksum_address.push(ch.to_ascii_uppercase());
} else {
checksum_address.push(ch);
}
}

checksum_address
}
}

/// An error obtained from trying to interpret a hex encoded string into an AccountId20
#[derive(Clone, Copy, Eq, PartialEq, Debug)]
#[allow(missing_docs)]
pub enum FromChecksumError {
BadLength,
InvalidChecksum,
InvalidPrefix,
}

impl Display for FromChecksumError {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
match self {
FromChecksumError::BadLength => write!(f, "Length is bad"),
FromChecksumError::InvalidChecksum => write!(f, "Invalid checksum"),
FromChecksumError::InvalidPrefix => write!(f, "Invalid checksum prefix byte."),
}
}
}

#[cfg(feature = "std")]
impl std::error::Error for FromChecksumError {}

impl Serialize for AccountId20 {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
serializer.serialize_str(&self.checksum())
}
}

impl<'de> Deserialize<'de> for AccountId20 {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
String::deserialize(deserializer)?
.parse::<AccountId20>()
.map_err(|e| serde::de::Error::custom(format!("{e:?}")))
}
}

impl core::fmt::Display for AccountId20 {
fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
write!(f, "{}", self.checksum())
}
}

impl core::str::FromStr for AccountId20 {
type Err = FromChecksumError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
niklasad1 marked this conversation as resolved.
Show resolved Hide resolved
if s.len() != 42 {
return Err(FromChecksumError::BadLength);
}
if !s.starts_with("0x") {
return Err(FromChecksumError::InvalidPrefix);
}
hex::decode(&s.as_bytes()[2..])
.map_err(|_| FromChecksumError::InvalidChecksum)?
.try_into()
.map(AccountId20)
.map_err(|_| FromChecksumError::BadLength)
}
}

#[cfg(test)]
mod test {
use super::*;

#[test]
fn deserialisation() {
let key_hashes = vec![
"0xf24FF3a9CF04c71Dbc94D0b566f7A27B94566cac",
"0x3Cd0A705a2DC65e5b1E1205896BaA2be8A07c6e0",
"0x798d4Ba9baf0064Ec19eB4F0a1a45785ae9D6DFc",
"0x773539d4Ac0e786233D90A233654ccEE26a613D9",
"0xFf64d3F6efE2317EE2807d223a0Bdc4c0c49dfDB",
"0xC0F0f4ab324C46e55D02D0033343B4Be8A55532d",
];

for key_hash in key_hashes {
let parsed: AccountId20 = key_hash.parse().expect("Failed to parse");

let encoded = parsed.checksum();

// `encoded` should be equal to the initial key_hash
assert_eq!(encoded, key_hash);
}
}
}
2 changes: 2 additions & 0 deletions core/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
//! Miscellaneous utility helpers.

mod account_id;
mod account_id20;
pub mod bits;
mod era;
mod multi_address;
Expand All @@ -21,6 +22,7 @@ use codec::{Compact, Decode, Encode};
use derive_where::derive_where;

pub use account_id::AccountId32;
pub use account_id20::AccountId20;
pub use era::Era;
pub use multi_address::MultiAddress;
pub use multi_signature::MultiSignature;
Expand Down
9 changes: 7 additions & 2 deletions signer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ secrecy = { workspace = true }
regex = { workspace = true, features = ["unicode"] }
hex = { workspace = true, features = ["alloc"] }
cfg-if = { workspace = true }
codec = { package = "parity-scale-codec", workspace = true, features = ["derive"] }
codec = { package = "parity-scale-codec", workspace = true, features = [
"derive",
] }
sp-crypto-hashing = { workspace = true }
pbkdf2 = { workspace = true }
sha2 = { workspace = true }
Expand All @@ -58,7 +60,10 @@ zeroize = { workspace = true }
bip39 = { workspace = true }
bip32 = { workspace = true, features = ["alloc", "secp256k1"], optional = true }
schnorrkel = { workspace = true, optional = true }
secp256k1 = { workspace = true, optional = true, features = ["alloc", "recovery"] }
secp256k1 = { workspace = true, optional = true, features = [
"alloc",
"recovery",
] }
keccak-hash = { workspace = true, optional = true }

# We only pull this in to enable the JS flag for schnorrkel to use.
Expand Down
Loading
Loading