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

Added Node Wallet support to BlockchainDataSource, along with a simple BoxLoader #164

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

K-Singh
Copy link
Contributor

@K-Singh K-Singh commented Apr 19, 2022

Would like a review, because implementing IUnspentBoxesLoader feels wrong due to needing an Address. Also no way to paginate boxes from the WalletAPI as far as I can tell, so had to pre-load all boxes which may be inefficient. I used to use ctx.getWallet quite a bit to get wallet boxes, so would like this functionality to still be supported some how.

@MrStahlfelge
Copy link
Member

Can you fix the tests?

@MrStahlfelge
Copy link
Member

We removed the wallet functionality from blockchain datasource completely: It is a functionality directly derived from restrictions of the node (it is not able to fetch unspend wallets for all addresses, but only for the own wallet), and it is not something the blockchain data is defining.

To work around this restriction without using the explorer you could introduce a NodeWalletDatasource implementing the getUnspentBoxesFor methods in a way that they actually return the node wallet's boxes.

However, I would completely dismiss this and work with the using an own loader only.

@K-Singh
Copy link
Contributor Author

K-Singh commented Apr 19, 2022

Can you fix the tests?

Yeah, didn't catch those before, will get a fix in tomorrow.

We removed the wallet functionality from blockchain datasource completely: It is a functionality directly derived from restrictions of the node (it is not able to fetch unspend wallets for all addresses, but only for the own wallet), and it is not something the blockchain data is defining.

To work around this restriction without using the explorer you could introduce a NodeWalletDatasource implementing the getUnspentBoxesFor methods in a way that they actually return the node wallet's boxes.

However, I would completely dismiss this and work with the using an own loader only.

Sure, I can create a NodeWalletDataSource specifically for this. I don't entirely understand what you mean by the last line though.

@MrStahlfelge
Copy link
Member

Sure, I can create a NodeWalletDataSource specifically for this. I don't entirely understand what you mean by the last line though.

The following: Instead of adapting the data source, you can just use the loader to fetch the data from the node. Instead of

 allBoxes = dataSource.getUnconfirmedUnspentWalletBoxes();

you do

allBoxes = (dataSource as NodeAndExplorerDataSourceImpl).getWalletApi().getUnconfirmedUnspentWalletBoxes();

No need to change the datasource itself, just use an own loader. You can do this in your project without changing appkit.

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