Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sdk: add client set_keys method #167

Closed
wants to merge 1 commit into from

Conversation

DanConwayDev
Copy link
Contributor

Description

Some applications want to connect relays and create subscriptions before a secret key has been decrypted and then later swap in the user's secret key.

This method makes doing so convenient.

Changelog notice

change keys used by an existing client,
z

All Submissions:

[ x ] I've signed all my commits
[ x ] I followed the contribution guidelines
[ x ] I ran make precommit before committing

Comment on lines +224 to +227
pub fn set_keys(&mut self, keys: &Keys) {
self.keys = keys.clone()
}

Copy link
Member

Choose a reason for hiding this comment

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

Thanks again!
Since the nostr Client can be shared across multiple threads, I think this method should not be a mutable.
A solution it's to wrap the Keys struct inside a Arc<RwLock<Keys>> (where the RwLock it's tokio::sync::RwLock).
So should be something like:

    pub async fn set_keys(&self, keys: &Keys) {
        let mut current_keys = self.keys.write().await;
        *current_keys = keys.clone();
    }

What do you think?
I expect that are many changes to do to (also in bindings crates). Let me know if you prefer that I'll do these changes.

Copy link
Contributor Author

@DanConwayDev DanConwayDev Sep 18, 2023

Choose a reason for hiding this comment

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

That's a good point. I can't think of a better solution than that which you proposed. I wasn't anticipating that this feature would require updates across the library and bindings.

Alternatively I could create a new Client when needed with the new Keys and a clone of the RelayPool. There might be some gotchas related to using the cloned relay pool.

Another option, is not to use Client at all. There is obviously a convenience and flexibility trade-off here. There are other benefits of using EventBuilder and RelayPool directly such as not needing to clone the RelayPool when direct access to it is needed. This might better fit my scenario.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the last proposal could be a solution for some scenarios.

But anyway, I think have a method to allow to change the Keys without change client would be useful.
As I said above, there are no problem if you prefer that I'll work on this change (wrap the keys in Arc<RwLock<Keys>>). Just let me know so I'll avoid to work on this if you already working on it.

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 client is actually really useful so thinking about it a bit more the last solution is not ideal.

I initially thought that your proposed solution would be adding additional complexity to your library but I can see that you have used a similar approach quite a lot.

if you prefer that I'll work on this change

That would be great thanks :)

Just let me know so I'll avoid to work on this if you already working on it.

You said that there is an impact on the bindings. I've not really looked at them so I think you're much better placed to make the change than me.

Once again, thanks very much.

Copy link
Member

Choose a reason for hiding this comment

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

Take a look to PR #170

You said that there is an impact on the bindings

My bad... yes, but less then expected (just a very little change on bindings).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just reviewed #170

@yukibtc yukibtc added this to the Release v0.25 milestone Sep 15, 2023
yukibtc added a commit that referenced this pull request Sep 18, 2023
yukibtc added a commit that referenced this pull request Sep 28, 2023
yukibtc added a commit that referenced this pull request Sep 28, 2023
@yukibtc yukibtc force-pushed the master branch 2 times, most recently from 927e34b to 5a88ee8 Compare September 30, 2023 14:05
@yukibtc yukibtc closed this in d8974c4 Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants