Skip to content

Conversation

OBorce
Copy link
Contributor

@OBorce 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

@@ -221,6 +223,13 @@ pub trait WalletStorageEncryptionWrite {
fn encrypt_seed_phrase(&mut self, new_encryption_key: &Option<SymmetricKey>) -> Result<()>;
}

pub trait WalletStorageReadWriteLocked: WalletStorageReadLocked + WalletStorageWriteLocked {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Comment on lines 1079 to 1081
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, hopefully it is more clear now

Comment on lines 1225 to 1316
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@OBorce OBorce force-pushed the refactor/wallet-generic-signer branch 3 times, most recently from b3bd09f to fd48654 Compare December 9, 2024 16:06
@OBorce OBorce force-pushed the refactor/wallet-async-signing branch from b05ce60 to dcefb1f Compare December 9, 2024 17:13
@OBorce OBorce force-pushed the refactor/wallet-generic-signer branch from fd48654 to 511bba3 Compare December 10, 2024 16:56
@OBorce OBorce force-pushed the refactor/wallet-async-signing branch 3 times, most recently from f0dcbbc to 6c8ab7c Compare December 11, 2024 09:07
@OBorce OBorce force-pushed the refactor/wallet-generic-signer branch from 3b76519 to 46cf688 Compare December 11, 2024 10:35
@OBorce OBorce force-pushed the refactor/wallet-async-signing branch from 6c8ab7c to 8d21f6a Compare December 11, 2024 10:35
@OBorce OBorce force-pushed the refactor/wallet-generic-signer branch 4 times, most recently from 69592f8 to f16c3e3 Compare February 3, 2025 10:47
@OBorce OBorce force-pushed the refactor/wallet-generic-signer branch from f13c1a1 to 82b71a8 Compare February 25, 2025 06:28
@OBorce OBorce marked this pull request as draft February 25, 2025 07:36
@OBorce OBorce force-pushed the refactor/wallet-generic-signer branch 3 times, most recently from 67609d5 to 220ace1 Compare February 28, 2025 09:58
Base automatically changed from refactor/wallet-generic-signer to master March 3, 2025 16:17
@OBorce OBorce force-pushed the refactor/wallet-async-signing branch 2 times, most recently from 8457baf to 9fbbe02 Compare May 24, 2025 15:39
@OBorce OBorce force-pushed the refactor/wallet-async-signing branch 3 times, most recently from ecb0292 to fee5884 Compare May 29, 2025 14:45
@OBorce OBorce marked this pull request as ready for review May 30, 2025 08:28
@OBorce OBorce force-pushed the refactor/wallet-async-signing branch from fee5884 to 59bd742 Compare July 15, 2025 14:52
- to make an easier integration with Ledger in the future
@OBorce OBorce force-pushed the refactor/wallet-async-signing branch from 59bd742 to 9099e90 Compare August 22, 2025 10:57
Comment on lines +158 to +161

pub fn local_rw_unlocked(&self) -> StoreLocalReadWriteUnlocked<B> {
StoreLocalReadWriteUnlocked::new(self.clone())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I've said during the daily, I guess I missed it during the initial review, but this looks like a nasty foot-gun:

  1. StoreLocalReadWriteUnlocked accumulates modifications inside the opaque operations collection, so it's possible to modify something, then read this value back (either via StoreLocalReadWriteUnlocked itself 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.
  2. It's possible to create multiple StoreLocalReadWriteUnlocked and 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.

  1. Passing a shared reference across an await point requires the referenced object to be Sync, which is not a reasonable restriction for a db transaction object. But this can be worked around using mutable references. E.g.
    • Signer methods 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_tx could be passed to the signer.
    • Inside private code the mutable references could be passed around directly, for simplicity.
  2. There is still the problem of sqlite::DbTx not being Send, this is because it contains a MutexGuard.
    At the first glance it looked like there should be no need to hold the mutex for the entire lifetime of DbTx and 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 a sqlite::DbTx when 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants