fix(wallet)!: make LoadParams implicitly satisfy Send

[evanlinjin]

Description

Make LoadParams implicitly satisfy Send. This will hopefully make AsyncWalletPersister easier to implement.

Refer to the conversation on Discord.

cc. @matthiasdebernardini

Notes to the reviewers

This is a breaking change, since we are tightening the bounds to some methods.

Changelog notice

• Change LoadParams to implicitly satisfy Send.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[oleonardolima]

Concept ACK, but unsure how to properly test it w/o an async persist implementation 😅.

[matthiasdebernardini]

@oleonardolima I'm working on it, but I can't get it to compile due to lifetime issues

evanlinjin#11

[LLFourn]

ACK 295b979

[LLFourn]

Concept ACK, but unsure how to properly test it w/o an async persist implementation 😅.

You can just create a function:

fn test_send<T: Send>() { }

and call it with the thing inside a test. May want to consider doing that here.

[evanlinjin]

@oleonardolima I'm working on it, but I can't get it to compile due to lifetime issues

evanlinjin#11

@matthiasdebernardini added to my mental todo list.

[matthiasdebernardini]

@evanlinjin @ValuedMammal helped me out and we got something going, but I still need to modify the AsyncWalletPersister trait so that I can pass the name of the wallet that I am querying like so

I was going to bring it up in our weekly meeting today to see if the rest are allright with it.

[evanlinjin]

@evanlinjin @ValuedMammal helped me out and we got something going, but I still need to modify the AsyncWalletPersister trait so that I can pass the name of the wallet that I am querying like so

I was going to bring it up in our weekly meeting today to see if the rest are allright with it.

In general it's a bad idea to introduce an implementation-specific parameter to a trait that is meant to be for multiple implementations. I think a better idea would be to introduce a generic type parameter to the persistence traits. I'll create a draft commit soon.

---

Edit: However, I'm wondering whether the better option would be for implementations to have a wrapped ...Persister type if they want some custom config.

I.e.

type NamedWalletConnection<'c> {
    wallet_name: String,
    conn: sqlx::Connection<'c>,
}

impl<'c> AsyncWalletPersister for NamedWalletConnection<'c> {
    // ... stuff ...
}

@matthiasdebernardini what do you think? Draft PR for ...Persister trait modifications incoming anyway.

[matthiasdebernardini]

Hey! I actually got it working like this

#[derive(Debug)]
pub struct Store {
    pool: PgPool,
    wallet_name: String,
}

impl Store {
    /// Construct a new [`Store`].
    pub async fn new(pool: Pool<Postgres>, wallet_name:String) -> Result<Self, Error> {
         Ok(Self { pool, wallet_name})
    }
}

and that seems to work quite well, so that wrapper is not needed for me. Though it might make it more obvious to the next person to implement this.

[notmandatory]

ACK 295b979

I'm not sure if @matthiasdebernardini still needs this change for his postgres store but tightening up the type bounds still looks like the right thing to do.