Use Wallet TransactionDatabase; Create WalletBuilder #581
Conversation
| _ => bail!("Unknown DB engine"), | ||
| }; | ||
|
|
||
| let mut wallets: Vec<Wallet> = Vec::new(); |
There was a problem hiding this comment.
Can we make something more reusable? A single place to convert a string into the struct, something like this
There was a problem hiding this comment.
That seems like a good idea for another PR.
| /// Wallet Database Error | ||
| type Err: Into<Error> + From<Error>; | ||
|
|
||
| pub trait ProofDatabase: Debug { |
There was a problem hiding this comment.
Since this is a new trait in practica? Can we introduce a Writable trait as well?
My idea is to have atomic updates to the database, meaning that all database updates need to instantiate a Writable object, then commit all changes or roll back.
There was a problem hiding this comment.
Can you explain more how this would work?
There was a problem hiding this comment.
Since this is a new trait in practica? Can we introduce a Writable trait as well?
I think this is a good idea but maybe should be a separate pr then adding the transactions. Just so we're not changing major functionality and adding features in the same one?
| #[cfg_attr(target_arch = "wasm32", async_trait(?Send))] | ||
| #[cfg_attr(not(target_arch = "wasm32"), async_trait)] | ||
| pub trait Database: Debug { | ||
| /// Wallet Database Error | ||
| type Err: Into<Error> + From<Error>; |
There was a problem hiding this comment.
Maybe for a separate PR but should maybe define a result type and use that everywhere.
| /// Get current Keyset counter | ||
| async fn get_keyset_counter(&self, keyset_id: &Id) -> Result<Option<u32>, Self::Err>; | ||
| async fn get_keyset_counter(&self, keyset_id: &Id) -> Result<Option<u32>, Error>; | ||
|
|
||
| /// Get when nostr key was last checked |
There was a problem hiding this comment.
Are these nostr methods necessary? Should we support this some other way?
There was a problem hiding this comment.
cdk/crates/cdk-cli/src/sub_commands/receive.rs
Lines 119 to 121 in dcb9ab3
Theyre used in the cli, but no they shouldn't be here. CLI or an application should have their own db for stuff like this
| /// Transaction ID | ||
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] | ||
| #[serde(transparent)] | ||
| pub struct TransactionId([u8; 32]); | ||
|
|
||
| impl TransactionId { | ||
| /// Create new [`TransactionId`] | ||
| pub fn new(ys: Vec<PublicKey>) -> Self { | ||
| let mut ys = ys; | ||
| ys.sort(); | ||
| let mut hasher = sha256::Hash::engine(); | ||
| for y in ys { | ||
| hasher.input(&y.to_bytes()); | ||
| } | ||
| let hash = sha256::Hash::from_engine(hasher); | ||
| Self(hash.to_byte_array()) | ||
| } | ||
|
|
||
| /// Get inner value | ||
| pub fn as_bytes(&self) -> &[u8; 32] { | ||
| &self.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
I know we've brought it up before but just to flag again. Do we think there is any value in adding an it like this to the spec? I think it could be used for SIGALL on melts.
There was a problem hiding this comment.
I think that's a good idea. Should I add it to NUT-00? cc: @callebtc
| pub enum TransactionDirection { | ||
| Incoming, | ||
| Outgoing, | ||
| } |
There was a problem hiding this comment.
How about in-place swaps? Meaning not as part of send or receive, but as a standalone wallet operation? Wallets may swap to consolidate proofs, or to change their distribution (more for this amount, less of that amount).
Since swaps can have a fee, they can affect the wallet balance. I think this should be accounted for in the tx list.
There was a problem hiding this comment.
On second thought, would that scenario be an "outgoing" transaction since the amount was deducted from the balance? The TransactionDirection enum is intended to keep the amount field unsigned.
I will add a fee field to the Transaction struct. Internal swaps would be identified by direction == TransactiongDirection::Outgoing && amount == fee.
There was a problem hiding this comment.
That sounds like an elegant solution.
Maybe mark it as "self-transfer" somehow (a special metadata field?) but otherwise outgoing tx makes sense.
| for y in ys { | ||
| hasher.input(&y.to_bytes()); | ||
| } |
There was a problem hiding this comment.
I'm not sure if only the the Ys should be part of this.
What if a wallet tries a tx, but it fails, then tries the same tx again with the same inputs, which succeeds? Intuitively, that should be shown as 2 txs, one failed one succeeded.
Maybe in addition to Ys it should hash the timestamp too?
There was a problem hiding this comment.
What would constitute failed transactions? I can't think of any failure reasons to persist a failed transaction.
There was a problem hiding this comment.
What would constitute failed transactions?
A bunch of things can lead to a tx to fail:
- send
- if sender needs to swap, but the mint is unreachable or otherwise errors out, so no swap can occur
- mint / melt
- if the quote reply from the mint is invalid (for example
expiryis in the past) - if the quote remains
UNPAIDafter theexpirytimestamp - if the quote calls are fine, but calls to
/mintor/meltfail (for any reason: auth, timeout, invalid response, etc) - if the amount to mint or melt is outside the
min/maxrange defined by the mint
- if the quote reply from the mint is invalid (for example
- receive
- if someone received a token, but swapping the proofs failed (maybe "failed" is not the right state for such a tx)
- all types of swap (as part of send / receive, or standalone)
- if the available inputs are not enough to cover the swap fees
- if the available inputs match exactly the needed fees, so there can be no outputs
I'm not saying the PR should account for all possible errors. But I do think get_transactions should be able to reflect attempted but failed txs, with a summary for failure reason.
I can't think of any (..) reasons to persist a failed transaction
Most (all?) mobile LN wallets show failed payments. Its a confirmation that the library actually tried to send / receive, but something went wrong. If get_transactions doesn't include the failed ones, app developers have to improvise and stub the failed txs, if they want to keep a good UX (on par with mobile LN wallets for example).
Another reason to store it is for troubleshooting issues with the mint. If the app has failed txs documented in get_transactions, the user can get the details (quote ID, timestamp, amount, etc) that can help the mint troubleshoot the issue. For mint / melt, the tx ID could be the quote ID.
There was a problem hiding this comment.
I don't think I've seen an app persist a transaction for any of these cases. Usually, they are alerts at the time of the user action. I don't see any reason to keep them around forever by storing them as a transaction (especially transient network issues).
Quotes are persisted separately from transactions and can be queried independently. I don't think it makes sense to put quote data in transactions.
There was a problem hiding this comment.
Of course a line has to be drawn to separate when does a transaction begin. Errors before that shouldn't reflect on the tx state, but if something goes wrong once a tx is started or in progress (for example, once the quote is pending), it seems reasonable to assume that will be reflected in a failed tx state.
On a more practical level, I don't think its possible to properly account for the proofs in the cashu wallet, without tracking for each of them, how they got in and if / how they got out. Dealing with "how they got in / out" opens the question for tx state, because the tx is often a multi-step process. For example, if a token was received, but no swap could be done, would that tx not appear in the history? What if it's a temporary mint connectivity issue and the wallet app (or CDK) retries to swap them later and it succeeds, would a history entry suddenly appear with a timestamp in the past?
IMO there is no way around dealing with failed transactions. If they're left out now, I think they'll have to be added later on.
There was a problem hiding this comment.
The transactions are more like add-on metadata than core components of the wallet. As we had decided earlier, the wallet is still basically a "bag of proofs." Therefore, failed transactions shouldn't need to be tracked.
The PR is now updated to add transactions after a successful mint, melt, send, or receive.
Description
Adds a
TransactionDatabasetrait for wallet databases to implement to store a history of wallet transactions.Notes to the reviewers
Suggested CHANGELOG Updates
CHANGED
WalletDatabasetoWalletProofDatabase.mint,melt,send, andreceivenow accept respective options struct.ADDED
WalletTransactionDatabaseandTransaction(handled by wallet functions)WalletBuilderREMOVED
FIXED
Checklist
just final-checkbefore committing