Breez signer #487
Breez signer #487
Conversation
roeierez
force-pushed
the
breez-signer
branch
from
4620bbf to
44d1dae
Compare
roeierez
force-pushed
the
breez-signer
branch
from
7dff500 to
37035fa
Compare
danielgranhao
left a comment
danielgranhao
left a comment
There was a problem hiding this comment.
Looking good! Just a few comments
roeierez
force-pushed
the
breez-signer
branch
from
4aade7e to
de3e35d
Compare
| // This forces users to sync from scratch to the sync server. | ||
| // Also delete the sync_initial_complete flag to force re-populating | ||
| // all payment metadata for outgoing sync using the new key. | ||
| name: "Clear sync tables for BreezSigner backward compatibility", |
There was a problem hiding this comment.
This needs a migration in the web storage as well.
| async fn sign_hash_schnorr( | ||
| &self, | ||
| hash: &[u8], | ||
| path: &DerivationPath, | ||
| ) -> Result<secp256k1::schnorr::Signature, SdkError> { | ||
| let derived = self | ||
| .key_set | ||
| .identity_master_key | ||
| .derive_priv(&self.secp, path) | ||
| .map_err(|e| SdkError::Generic(e.to_string()))?; | ||
| let message = | ||
| Message::from_digest_slice(hash).map_err(|e| SdkError::Generic(e.to_string()))?; | ||
| let keypair = derived.private_key.keypair(&self.secp); | ||
| Ok(self.secp.sign_schnorr(&message, &keypair)) | ||
| } |
There was a problem hiding this comment.
Should these be forwarded to the Spark signer as well, where possible? The sign_schnorr implementation here uses auxiliary randomness, while the spark signer implementation doesn't. Perhaps the Spark signer can sign based on a derivation path as well? Not sure what's the best approach here, since the signer will be split anyway. Perhaps the best thing is to not forward anything to the Spark signer at all.
There was a problem hiding this comment.
added a flag to whether use randomness or not. For spark used without randomness for nostr with (as it was before this change).
There was a problem hiding this comment.
I'm not sure we need/should be using schnorr signing without aux rand. I checked, and it seems I introduced it, but probably was a mistake? If we see that all itests are green with rand, I would go ahead and and always use it.
As far as I can see, the JS SDK always uses aux rand.
| let identity_path = "m" | ||
| .parse() | ||
| .map_err(|e: bitcoin::bip32::Error| SignerError::Generic(e.to_string()))?; |
There was a problem hiding this comment.
This loses the KeySet functionality. It seems wrong to me that this SparkSigner passes the request to the BreezSigner, which passes it to DefaultSigner. Perhaps BreezSigner should have a SparkSigner, and the SparkSigner should have a DefaultSigner? Or maybe DefaultSigner shouldn't be used at all?
There was a problem hiding this comment.
@JssDWt I am not sure I follow here, let me explain the rational I use here:
- BreezSigner is the signer breez sdk needs. It currently uses under the hood an instance of spark DefaultSigner
- All other signers (rtsync, nostr, spark) depends on BreeSigner as adapters. Since BreezSigner eventually might be passed to the sdk by the user we must have SparkSigner depends on it.
The"under the hood" spark DefaultSigner that is used by BreezSigner is initialized with the right keyset so I don't see how we loose the keyset functionality.
LMK if I am missing anything here.
There was a problem hiding this comment.
I see, the dependency structure makes sense then.
An issue here is the identity key has a different derivation path for different keysets. Passing derivation path m is not backward compatible with the currently used keysets.
There was a problem hiding this comment.
As discussed offline, this is backward compatible as we do use the right keyset in the underline spark signer.
| "DELETE FROM sync_outgoing; | ||
| DELETE FROM sync_incoming; | ||
| DELETE FROM sync_state; | ||
| UPDATE sync_revision SET revision = 0; | ||
| DELETE FROM settings WHERE key = 'sync_initial_complete';", |
There was a problem hiding this comment.
Confirming: We delete all remote data. So sync depends on data being present locally to resync with the sync_initial_complete flag. Someone that doesn't have local persistent storage between sessions will lose all metadata. Any pending outgoing changes may be missed/lost. Is that intended?
There was a problem hiding this comment.
Unless I understand it wrong, only restore will will not sync in any metadata. Any upgrade to an existing instance will sync out everything (including pending changes as all local data is re-synced).
There was a problem hiding this comment.
Right, agreed.
So if someone doesn't have persistent local storage, they lose their payment metadata.
(cherry picked from commit 2339157d7669dd451437fec07e1edbfc0833bbda)
(cherry picked from commit 26723fe6f72be6ca7b9c0d0ab097bfe592434c29)
This comment was marked as outdated.
This comment was marked as outdated.
Refactors the signing architecture by introducing a unified
BreezSignertrait and specialized adapter implementations:BreezSignerImpl: Core signer implementationSparkSigner: Adapter for spark-wallet operationsNostrSigner: Adapter for Nostr event signingRTSyncSigner: Adapter for real-time sync signing/encryption