conflicting design in ed25519 and sr25519 ss58check

[atenjin]

from_ss58checkandto_ss58check is conflicting for ed25519::Public and sr25519::Public:
in core/primitives/src/ed25519.rs:

// for `ed25519::Public` the `from_ss58check` and `to_ss58check` is a function for `Public`
#[cfg(feature = "std")]
impl Public {
	/// Some if the string is a properly encoded SS58Check address.
	pub fn from_ss58check(s: &str) -> Result<Self, PublicError> {
		let d = s.from_base58().map_err(|_| PublicError::BadBase58)?;	// failure here would be invalid encoding.
		if d.len() != 35 {
			// Invalid length.
			return Err(PublicError::BadLength);
		}
		if d[0] != 42 {
			// Invalid version.
			return Err(PublicError::UnknownVersion);
		}
// notice this, it just put all data `d[0..33]` into blake2b
		if d[33..35] != blake2_rfc::blake2b::blake2b(64, &[], &d[0..33]).as_bytes()[0..2] {
			// Invalid checksum.
			return Err(PublicError::InvalidChecksum);
		}
		Ok(Self::from_slice(&d[1..33]))
	}

	/// Return the ss58-check string for this key.
	pub fn to_ss58check(&self) -> String {
		let mut v = vec![42u8];
		v.extend(self.as_slice());
		let r = blake2_rfc::blake2b::blake2b(64, &[], &v);
		v.extend(&r.as_bytes()[0..2]);
		v.to_base58()
	}
}

we can notice that

• for ed25519::Public the from_ss58check and to_ss58check is a function for Public
• the checksum is using d[0..33]

but there is no from_ss58check and to_ss58check funtion for sr25519::Public, instead, it the ss58 is from trait core/primitives/src/crypto.rs:L201:Ss58Codec

#[cfg(feature = "std")]
pub trait Ss58Codec: Sized {
	/// Some if the string is a properly encoded SS58Check address.
	fn from_ss58check(s: &str) -> Result<Self, PublicError>;
	/// Some if the string is a properly encoded SS58Check address, optionally with
	/// a derivation path following.
	fn from_string(s: &str) -> Result<Self, PublicError> { Self::from_ss58check(s) }
	/// Return the ss58-check string for this key.
	fn to_ss58check(&self) -> String;
}

and it has an impl for all struct

#[cfg(feature = "std")]
const PREFIX: &[u8] = b"SS58PRE";

#[cfg(feature = "std")]
fn ss58hash(data: &[u8]) -> blake2_rfc::blake2b::Blake2bResult {
	let mut context = blake2_rfc::blake2b::Blake2b::new(64);
	context.update(PREFIX);
	context.update(data);
	context.finalize()
}
#[cfg(feature = "std")]
impl<T: AsMut<[u8]> + AsRef<[u8]> + Default + Derive> Ss58Codec for T {
	fn from_ss58check(s: &str) -> Result<Self, PublicError> {
		let mut res = T::default();
		let len = res.as_mut().len();
		let d = s.from_base58().map_err(|_| PublicError::BadBase58)?; // failure here would be invalid encoding.
		if d.len() != len + 3 {
			// Invalid length.
			return Err(PublicError::BadLength);
		}
		if d[0] != 42 {
			// Invalid version.
			return Err(PublicError::UnknownVersion);
		}
                // notice this, the len is 32 for `Public`, thus, d[0..33], same to `ed25519::Public`
               // but `ss58hash` would hash with a `PREFIX` !
		if d[len+1..len+3] != ss58hash(&d[0..len+1]).as_bytes()[0..2] {
			// Invalid checksum.
			return Err(PublicError::InvalidChecksum);
		}
		res.as_mut().copy_from_slice(&d[1..len+1]);
		Ok(res)
	}

	fn to_ss58check(&self) -> String {
		// ...
	}

	fn from_string(s: &str) -> Result<Self, PublicError> {
		// ...
	}
}

we can see that:

• sr25519 do not has ss58xxx for the struct Public itself, but use the trait Ss58Codec to provide them, however, the trait Ss58Codec satisfy both sr25519::Public and ed25519::Public!
• though use same d[0..33] to hash, Ss58Codec use a PREFIX, thus, the checksum is not same for sr25519::Public and ed25519::Public

I do a test like this:

    {
        use substrate_primitives as primitives;
        use substrate_primitives::crypto::UncheckedInto;
        use hex_literal::{hex, hex_impl};
// a random 32 bytes hex string
        let alice = hex!["00308187439ac204df9e299e1e54a00000000bf348e03dad679737c91871dc53"];
// I use same hex string to generate different `Public`
        let pub1: primitives::ed25519::Public = alice.unchecked_into();
        let pub2: primitives::sr25519::Public = alice.unchecked_into();

        let addr1 = pub1.to_ss58check();
        println!("from ed25519::Public, addr1:{:}", addr1);
        {
            use substrate_primitives::crypto::Ss58Codec;
            let addr2 = pub2.to_ss58check();
            println!("from sr25519::Public, addr2:{:}", addr2);

            let addr3 = Ss58Codec::to_ss58check(&pub1); // use trait `Ss58Codec` for `ed25519::Public`
            println!("from ed25519::Public, addr3:{:}", addr3);
        }
    }

and the result like this:

from ed25519::Public, addr1:5C4xGQZwoNEM5mdk2U3vJbFZPr6ZKFSiqWnc9JRDcJ3w2x5D  # notice the checksum "2x5D "
from sr25519::Public, addr2:5C4xGQZwoNEM5mdk2U3vJbFZPr6ZKFSiqWnc9JRDcJ3w334p # notice the checksum "334p"
from ed25519::Public, addr3:5C4xGQZwoNEM5mdk2U3vJbFZPr6ZKFSiqWnc9JRDcJ3w334p # notice the checksum "334p"

the trait Ss58Codec function name is same to ed25519::Public

I think the trait Ss58Codec should also use for ed25519::Public, use PREFIX to hash as the checksum, remove function from_ss58check and to_ss58check in ed25519::Public.

and on the other hand, the js-sdk should also be modified for this!

if ed25519::Public not use same checksum for sr25519::Public, it should not use same function name.

[marcio-diaz]

Thanks for the detailed explanation @jkingdom. Closed by #2355

[atenjin]

Hi, we notice that the front end has not changed yet for this modification:

• polkadot-js https://github.com/polkadot-js/common/blob/master/packages/keyring/src/address/encode.ts
• oo7 https://github.com/paritytech/oo7/blob/master/packages/oo7-substrate/src/ss58.js#L15

hope to fix it soon