Add 20-byte account id to subxt_core

[pkhry]

Description:

see #1576. This mr just moves the AccountId20 implementation to subxt_core and adds serde's Serialize and Deserialize implementations for it.

closes #1576

[niklasad1]

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 :)

[pkhry]

is it really that important?

[niklasad1]

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

[niklasad1]

do we need the keccak-hash depdendency now when it's dependency in subxt-core?

[jsdw]

From my prev comment, let's remove the subxt-core dep here :)

[niklasad1]

LGTM, nice to have this for sure.

I guess when the new metadata v16 then the codegen can be smart enough to use AccountId20 instead of AccountId32 from ethereum-style metadata/chains?

[pkhry]

LGTM, nice to have this for sure.

I guess when the new metadata v16 then codegen can smart enough to use AccountId20 instead of AccountId32 from ethereum-style metadata/chains?

We need to know the path for the accountId20 to provide a substitution in this case, If anyone can tag in with it we can add it to the substitution rules inside codegen/lib.

EDIT: added substitution inside codegen for fp_account::AccountId20 and updated frontier example accordingly

[niklasad1]

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...

[jsdw]

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 :)

[jsdw]

Let's avoid importing subxt-core here, and instead copy the pattern from eg sr25519.rs where we have a standalong PublicKey struct and then in the subxt_compat section we pull in subxt-core and have the relevant From impls etc to make PublicKey play nice with subxt's AccountId20 :)

[pkhry]

what about the tests for eth? they will be enable only if "subxt" feature is also enabled

[jsdw]

Ah the tests are no problem; currently we just assume the default features are enabled for them, which allows subxt-core anyways. I just think that we can avoid needing subxt-core by default for "eth" signing to fall in line with the other algorithms :)

[jsdw]

Looks good to me! Only things to do are fixing the CI issues and removing the subxt-core dep from subxt-signer's eth feature by following the same pattern as sr25519.rs etc :)

[pkhry]

issues with subxt-macro related, to clippy

[niklasad1]

issues with subxt-macro related, to clippy

Just add a lint #![allow(clippy::manual_unwrap_or_default)] in macros then, it's the darling dependency that causes that and just silly thing.....

I guess we could file an issue in darling but still quite opinionated lint IMHO :)

[jsdw]

Ah this isn't quite what I had in mind; I was hitnking you could do something more like sr25519.rs and have eg:

/// The public key for an [`Keypair`] key pair. This is equivalent to an `AccountId20`.
pub struct PublicKey(pub [u8; 20]);

pub struct Keypair(..)

// Impl here; not gated by subxt-compat feature:
impl Keypair {
    // ...
}

#[cfg(feature = "subxt")]
mod subxt_compat {
    use super::*;

    // Only in this block do we now need to import subxt-core to
    // describe how to convert our PublicKey above into an AccountId20:
    use subxt_core::utils::AccountId20;

    impl From<PublicKey> for AccountId20 {
        fn from(value: PublicKey) -> Self {
            value.to_account_id()
        }
    }

    impl PublicKey {
        /// A shortcut to obtain an [`AccountId20`] from a [`PublicKey`].
        /// We often want this type, and using this method avoids any
        /// ambiguous type resolution issues.
        pub fn to_account_id(self) -> AccountId20 {
            AccountId20(self.0)
        }
    }

    impl<T: Config> SignerT<T> for Keypair
    where
        T::AccountId: From<PublicKey>,
        // looks like T::Address is also AcocuntId20 so the one From impl above works,
        // but this allows other address types to be used as long as there's a way to convert:
        T::Address: From<PublicKey>, 
        // sameish: we just use the Signature type above so I think this Just Works, but
        // also leaves door open for people to use other Signature types in their Config as 
        // long as they provide a From impl:
        T::Signature: From<Signature>,
    {
        fn account_id(&self) -> T::AccountId {
            self.public_key().into()
        }

        fn address(&self) -> T::Address {
            self.public_key().into()
        }

        fn sign(&self, signer_payload: &[u8]) -> T::Signature {
            self.sign(signer_payload).into()
        }
    }
}

[pkhry]

Ah, ok, will push the change shortly

[pkhry]

updated in line with the suggestion

[jsdw]

Ah yeah ok; it's a bit different from sr25519 etc because the public key isn't directly the bytes we want for the acocuntId. I like this wrapper type though; I suppose it could just contain the ecdsa::PublicKey at this point, but I'm not too bothered either way :)

[jsdw]

LGTM, nice job @pkhry!

[muraca]

Lovely, thank you!

[jsdw]

Next time we should make sure the CI is passing before merging (which in this case means fixing the failing test); just incase something else was broken in the other CI steps that we missed :)