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

Add a new API Wallet::new_with_online(). #16

Conversation

monaka
Copy link
Contributor

@monaka monaka commented Dec 16, 2022

I'm developing RESTful http interface for RGB-Lib.
This PR is for my app.

As you might know, web frameworks tends to be stateless.
So I want to keep my Wallet instance over sessions.
But as far as I trided, the instance of Wallet structure couldn't store in reference count traits like Rc<> or Arc<>.

I also tried instancing Wallet per session.
Results were "panicked" because of thread generation in _go_online().

This PR enables to inject Online object when a Wallet instance is created.
This approach is kept a backward compatibility. All tests are passed.

Signed-off-by: Masaki Muranaka [email protected]

@monaka monaka force-pushed the pr-add-wallet-new_with_online branch from 9ba4dd2 to 8650d6c Compare December 16, 2022 00:11
@monaka monaka force-pushed the pr-add-wallet-new_with_online branch from 8650d6c to 63e4180 Compare December 16, 2022 00:23
@monaka
Copy link
Contributor Author

monaka commented Dec 16, 2022

I found lint errors by clippy. But they are not caused by my patches.

@monaka monaka marked this pull request as draft December 16, 2022 01:11
@monaka
Copy link
Contributor Author

monaka commented Dec 16, 2022

After tracking #17, I realized another approach is required to fix my pain point.
I close this PR as "won't fix".

@monaka monaka closed this Dec 16, 2022
@monaka monaka deleted the pr-add-wallet-new_with_online branch December 16, 2022 12:04
@zoedberg
Copy link
Collaborator

Hi @monaka,

I believe this API will not solve your issues.

First, it cannot work as it has been implemented. The Online object is not an object meant to be manually constructed or shared across different wallets. Its purpose is to prevent lib users to call APIs that require the wallet to have called go_online first.

Second, the issue you're facing in shiro-backend, as I tried to explain here, comes from a very clear limitation: it is not possible to create more than a wallet instance on the same data directory (in the same process in your case, but also using different processes you will end up with other painful bugs).

So to fix the issues you’re facing you need to store the wallet instance and reuse it. I know this can be frustrating, but we cannot do much until RGB-WG/rgb-node#222 gets addressed.

@monaka
Copy link
Contributor Author

monaka commented Dec 17, 2022

@zoedberg Thank you for you suggestion.
I got the reason why this PR is useless before closing this.
Your comment makes me more clear.

And in fact, I tried to share an instance of struct Wallet before.
It was failed by another error message.
I'm going to create a tiny crate that reproduces errors.
I'll share it within tomorrow.

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.

2 participants