Split Secret Manager traits #1756
Conversation
DaughterOfMars
force-pushed
the
feat/split-secret-manage
branch
from
556bfea to
8eebdef
Compare
| Ok(m.generate(&options).await?) | ||
| } | ||
| #[cfg(feature = "private_key_secret_manager")] | ||
| SecretManager::PrivateKey(_) => todo!(), |
There was a problem hiding this comment.
So all of these are here because I'm not sure what we want to do if you try to generate multiple keys using a private key secret manager (which can only ever give you one key). Should we give that key, even if it doesn't match the provided config? Or return an error?
| Ok(m.generate(&options).await?) | ||
| } | ||
| #[cfg(feature = "private_key_secret_manager")] | ||
| SecretManager::PrivateKey(_) => todo!(), |
| Ok(m.generate(&options).await?) | ||
| } | ||
| #[cfg(feature = "private_key_secret_manager")] | ||
| SecretManager::PrivateKey(_) => todo!(), |
| type Error: std::error::Error + Send + Sync; | ||
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct PublicKeyOptions { |
There was a problem hiding this comment.
What's the reason again to have this instead of Bip44 directly?
There was a problem hiding this comment.
Bip44 is not our type, but we could also make it a PublicKeyOptions(Bip44)?
There was a problem hiding this comment.
I've thought about changing it, but of course then there's the mismatch between generating a single key and multiple
There was a problem hiding this comment.
Okay, but should we maybe name it Bip44PublicKeyOptions to make its purpose clearer?
There was a problem hiding this comment.
Hmm..I'll think about it.
There was a problem hiding this comment.
It's not very important, so you can resolve whether you do or not
| match block_indexes.get(&required_address) { | ||
| // If we already have an [Unlock] for this address, add a [Unlock] based on the address type | ||
| Some(block_index) => match required_address { | ||
| Address::Ed25519(_) | Address::ImplicitAccountCreation(_) => { |
There was a problem hiding this comment.
Can this actually still be Address::ImplicitAccountCreation? It was converted in L227, right?
| } | ||
| } | ||
| impl<T> SecretManageExt for T {} |
| bip32_change: chain.change.harden().into(), | ||
| bip32_index: chain.address_index.harden().into(), |
There was a problem hiding this comment.
Before this was part of the remainder, but now it's just provided to this function which I think can be wrong if the remainder address isn't an Ed25519 address
So if remainder address != Ed25519, return None for the remainder_bip32 and below only go in this part if let Some(remainder_output) = remainder_output { if it's Some
There was a problem hiding this comment.
Since you understand this better, would you push the change?
There was a problem hiding this comment.
I think it depends on #1756 (comment), if we bring this back, then we can also do this part different
| /// The chain derived from seed, only for ed25519 addresses | ||
| #[serde(with = "option_bip44", default)] | ||
| pub chain: Option<Bip44>, |
There was a problem hiding this comment.
So do we bring this back for signing with multiple different addresses in inputs?
| PublicKeyOptions::new(bip.coin_type) | ||
| .with_account_index(bip.account) | ||
| .with_internal(bip.change != 0) | ||
| .with_address_index(bip.address_index), |
There was a problem hiding this comment.
Can now just be PublicKeyOptions::from(bip), also in more places
|
Working on a better solution. |
Description of change
This PR splits the
SecretManagetrait and refactors the trait to unify the essential functions for a wallet.Links to any relevant issues
Closes #1490