Certs_check, derive commands (without verification), + some other changes

[brh28]

• Cert_check verifies the card against the Factory public key (Tapsigner only)
• from_status constructor for tapsigner
• Reorg of commands.rs to group request and responses by command type

[notmandatory]

Great job figuring out how to verify the root certs! I also like the command/response reorg. Three things that I'd like you to do before merging are:

1. replace the assert! with an Err() in certs_check
2. remove replaced commented out code (in lib.rs)
3. be sure to do a cargo fmt

Thanks!

[notmandatory]

Instead of an assert! how about adding new Error type CertCheck(String) and use it here like:

Suggested change

	).is_ok());
	).map_err(|e| Error::CertCheck(e.to_string()))?;

Even though this is a fatal issue, we if just crash the app we don't have a chance to handle the error and tell the user what's going on.

[notmandatory]

Here's an idea for how to use const values for this. But for this PR I don't mind leaving it as is so we can think about it some more.

/// Published Coinkite factory root keys.
const PUB_FACTORY_ROOT_KEY:&str = "03028a0e89e70d0ec0d932053a89ab1da7d9182bdc6d2f03e706ee99517d05d9e1";
/// Obsolete dev value, but keeping for a little while longer.
const DEV_FACTORY_ROOT_KEY:&str = "027722ef208e681bac05f1b4b3cc478d6bf353ac9a09ff0c843430138f65c27bab";

pub enum FactoryRootKey {
    Pub(PublicKey),
    Dev(PublicKey),
}

impl TryFrom<PublicKey> for FactoryRootKey {
    type Error = Error;

    fn try_from(pubkey: PublicKey) -> Result<Self, Self::Error> {
        if pubkey.serialize().to_hex().eq(PUB_FACTORY_ROOT_KEY) {
            Ok(FactoryRootKey::Pub(pubkey))
        } else if pubkey.serialize().to_hex().eq(DEV_FACTORY_ROOT_KEY) {
            Ok(FactoryRootKey::Dev(pubkey))
        } else {
            Err(Error::CertCheck(
                "Root cert is not from Coinkite. Card is counterfeit.".to_string(),
            ))
        }
    }
}

impl Debug for FactoryRootKey {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        match &self {
            FactoryRootKey::Pub(pk) => {
                write!(f, "FactoryRootKey::Pub({:?})", pk)
            }
            FactoryRootKey::Dev(pk) => {
                write!(f, "FactoryRootKey::Dev({:?})", pk)
            }
        }
    }
}

[notmandatory]

If we go with a FactoryRootKey the end of this function becomes:

Suggested change

	}
	pubkey.try_into()

And returns Result<FactoryRootKey,Error>. But as mentioned above we don't need to decide for this PR, so unless you love my Enum idea we can leave this as is for now.

[notmandatory]

I also tested with my TapSigner and SATsCard and cert verification works for me.

[brh28]

Added the FactoryRootKey struct as you suggested, and added a "name" function to return a displayable string.

Combined the certs_check into the SharedCommands. The only logic difference between cards is the message_bytes, which is defined in the "signed_message" function. I don't have an older versioned SatsCard (0.9.0), so haven't been able to test that condition.

Created an IncorrectSignature type for the Error enum. Would you prefer an error type for each command? (i.e rename to CertsCheck)

[notmandatory]

ACK f8bf621

Looks good, but we're both going to have to be better about making cleaner commit histories on future PRs. But that should be easier with recent code reorgs behind us.

[brh28]

ACK f8bf621

Looks good, but we're both going to have to be better about making cleaner commit histories on future PRs. But that should be easier with recent code reorgs behind us.

Sorry about that, I usually use the "Squash & Merge" option on GitHub, so I have a bad habit about cleaning my commit history.