Conversation
OBorce
commented
Nov 28, 2024
- Make the Signer trait use async functions to enable future support for the Ledger wallet integration.
- Change to use a local object that stores any new changes in memory instead of a DB transaction so it can used more easily in async contexts than a reference
wallet/storage/src/lib.rs
Outdated
| fn encrypt_seed_phrase(&mut self, new_encryption_key: &Option<SymmetricKey>) -> Result<()>; | ||
| } | ||
|
|
||
| pub trait WalletStorageReadWriteLocked: WalletStorageReadLocked + WalletStorageWriteLocked {} |
There was a problem hiding this comment.
This change to the hierarchy is confusing. If I understand correctly you needed new storage type without a reference to an actual storage. But why introducing new traits if you could implement previous for a new type?
wallet/src/wallet/mod.rs
Outdated
| // In case of an error reload the keys in case the operation issued new ones and | ||
| // are saved in the cache but not in the DB |
There was a problem hiding this comment.
The grammar is broken in this sentence and the meaning is not super clear.
Also, since it's a copy-paste, the original should be updated too.
There was a problem hiding this comment.
updated, hopefully it is more clear now
wallet/src/wallet/mod.rs
Outdated
| let acc = accounts | ||
| .remove(&account_index) | ||
| .ok_or(WalletError::NoAccountFoundWithIndex(account_index))?; | ||
|
|
||
| let (acc, res) = f(acc).await; | ||
|
|
||
| accounts.insert(account_index, acc); | ||
| Ok(res) |
There was a problem hiding this comment.
I suppose this is a workaround for that weird lifetime-related issue you posted in the chat recently.
But I thought you've managed to solve it by boxing some futures. So it didn't work after all?
There was a problem hiding this comment.
The boxed future is used in the Synced Controller, this was a prior workaround for the lifetimes to not pass the account as a reference but as a value. I have not tried if a boxed future will solve this as well or not, but also not sure how expensive boxed futures are?
b3bd09f to
fd48654
Compare
b05ce60 to
dcefb1f
Compare
fd48654 to
511bba3
Compare
f0dcbbc to
6c8ab7c
Compare
3b76519 to
46cf688
Compare
6c8ab7c to
8d21f6a
Compare
69592f8 to
f16c3e3
Compare
f13c1a1 to
82b71a8
Compare
67609d5 to
220ace1
Compare
8457baf to
9fbbe02
Compare
ecb0292 to
fee5884
Compare
fee5884 to
59bd742
Compare
59bd742 to
9099e90
Compare
wallet/storage/src/internal/mod.rs
Outdated
|
|
||
| pub fn local_rw_unlocked(&self) -> StoreLocalReadWriteUnlocked<B> { | ||
| StoreLocalReadWriteUnlocked::new(self.clone()) | ||
| } |
There was a problem hiding this comment.
As I've said during the daily, I guess I missed it during the initial review, but this looks like a nasty foot-gun:
StoreLocalReadWriteUnlockedaccumulates modifications inside the opaqueoperationscollection, so it's possible to modify something, then read this value back (either viaStoreLocalReadWriteUnlockeditself or its "parent" transaction) and get the old value instead of the modified one.
If we want to go this route, we should probably do it on a lower level, where the underlying key-value store is available, so that we can store deltas instead of opaque operations, which then could be applied on the fly when reading.- It's possible to create multiple
StoreLocalReadWriteUnlockedand the result of their application is unpredictable.
P.S. I've also played with the code for some time trying to come up with something else.
- Passing a shared reference across an
awaitpoint requires the referenced object to beSync, which is not a reasonable restriction for a db transaction object. But this can be worked around using mutable references. E.g.Signermethods could accept the tx by value:db_tx: impl WalletStorageReadUnlocked + Send;- We could have
impl<T> WalletStorageReadLocked for &mut T where T: WalletStorageReadLocked, so that&mut db_txcould be passed to the signer. - Inside private code the mutable references could be passed around directly, for simplicity.
- There is still the problem of
sqlite::DbTxnot beingSend, this is because it contains aMutexGuard.
At the first glance it looked like there should be no need to hold the mutex for the entire lifetime ofDbTxand that we could lock the mutex inside its individual methods. Unfortunately, this didn't work, as multiple tests started failing in the wallet due nested sqlite transactions being created. I didn't investigate it much, but it looks like there is some code in the wallet that can create asqlite::DbTxwhen another one has not been dropped yet, so it depends on the presence of the mutex lock.
(Btw, we don't have a mutex inside lmdb transaction objects, so it looks like the backends are not interchangeable, unless I'm missing something.)
At this moment it seems to me that the correct approach would be to have a separate set of storage-related traits (Backend, transactions etc) where all the methods are async. And then have an implementation of AsyncBackend for sqlite. The implementation will be able to use tokio::Mutex whose MutexGuard is Send.
f21e82b to
75ff652
Compare
- to make an easier integration with Ledger in the future
- return the db_tx from async operations in order for it to be committed or rolled back properly
75ff652 to
96016cf
Compare
|
superseded by #1976 |