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

Extract stateless functions into wallet library module #202

Merged
merged 5 commits into from
Aug 17, 2021

Conversation

rishflab
Copy link
Member

@rishflab rishflab commented Jul 30, 2021

Sorry for tagging everyone as reviewer but It kind of does concern everyone.

NOTE: probably easier to checkout the branch and look at it in your IDE

The goal of this commit/branch is to extract wallet
functionality into a module so it can moved into the baru repo The
motivation for this refactor is so the platform team can upgrade the
protocols to use PSET's. This should also take some workload away
from from product team.

The stuff that will be moved into the baru repo is in the baru-wallet crate

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Overall a good starting point. Try to push for more value-based APIs where possible instead of depending on traits.

Additionally, I think it should actually be the responsibility of baru to provide "backends" like esplora. So maybe the Wallet trait can be trimmed down to a Blockchain backend (similar to bdk) and then we define functionality composed on top of this trait?

Also, I think baru should offer an actual Wallet struct that the extension can re-use that holds state like the keys, and UTXOs. When given a Backend, the wallet can then sync the UTXOs and perform IO-less stuff like signing a loan transaction etc.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

This is coming together nicely.

Take the following with a grain of salt because maybe it is not fully-possible but I was thinking we could do something like:

  1. Have Wallet store a list of unspent transaction outputs. Whatever data structure we store, it should be everything we need to know about our UTXOs. In the future, we can abstract this into some kind of backend rather then a just an in-memory list.
  2. Have a Wallet::sync function that looks roughly like this:
impl Wallet {
    async fn sync(&mut self, backend: &impl Backend) {
        let utxos = backend.fetch_utxos(self.address());

         // for utxo in utxos, fetch transaction and compute txout ...

         // self.txouts = 
    }
}
  1. All APIs like extract_loan can now be synchronous functions on Wallet because Wallet owns most of the data necessary to do the task (txouts, blinding key etc)

Some notes about this design:

  • It should allow the Backend trait to be minimal and only concern itself with IO.
  • baru can provide an implementation of Backend for esplora but given that specific need of caching using the browser's cache etc, I would probably not bother and leave that within the extension for now.
  • Passing the Backend as a parameter to sync allows Wallet to remain simple and without type parameters and allows for easier testing using dummy backends.
  • Having Wallet own state like TxOuts makes all the functions like extract_loan quite easy to use.

On top of all this, the extension should probably define its own Wallet struct that wraps the one provided by baru and carry around the state for usdt_asset_id, chain, etc.


I recognize this may seem contradicting to my previous review. However, for this design to be possible, it was important to first split things into pure functions (value-based). Now that this is the case, we can easily re-compose everything that is there by bundling the necessary state for these functions up in the Wallet struct.

@rishflab
Copy link
Member Author

rishflab commented Aug 2, 2021

  • Having Wallet own state like TxOuts makes all the functions like extract_loan quite easy to use.

good idea. another benefit is the caller won't have to pass around the touts in their app

@rishflab rishflab requested a review from thomaseizinger August 5, 2021 07:56
@rishflab rishflab marked this pull request as ready for review August 5, 2021 07:56
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Looking good, I think we are almost there!

To summarize:

  • Add a coin-selection API to Wallet
  • Add a signing API to Wallet

That should allow you to remove the utxos() accessor on Wallet and greatly reduce the duplication between the exposed wallet functions.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

The diff here is quite big :/

What is your appetite for making it smaller?
I think there might be a reasonably easy way of doing that.

  1. Make a new branch from master.
  2. First commit: Bump to baru version 0.3.
  3. Second commit: Introduce EsploraClient as a struct that owns the base_url
  4. Third commit: Add a git dependency on baru that uses the new wallet and deletes all the old stuff.

Given that the overall structure of the #[wasm_bindgen] functions etc didn't change much, (4) should be relatively straight forward and should result in a much smaller diff :)

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nice work! This diff is actually reviewable now :)

})
.collect()
}
pub use baru::BalanceEntry;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go to the very top to the other exports.

@thomaseizinger
Copy link
Contributor

You probably want to rebase your stuff onto #233 and drop the commit where you upgrade to baru 0.3.

@da-kami
Copy link
Member

da-kami commented Aug 12, 2021

You probably want to rebase your stuff onto #233 and drop the commit where you upgrade to baru 0.3.

I think the only commit that can be dropped is 95fade9 (the one that upgrades bobtimus)

The wallet upgrade should please stay - I don't touch that in my PR, but kept the deprecated API.

@rishflab rishflab changed the base branch from master to upgrade-bobtimus-baru August 12, 2021 07:30
@bonomat bonomat force-pushed the upgrade-bobtimus-baru branch from fa23aeb to 1cc5d7e Compare August 14, 2021 06:22
Base automatically changed from upgrade-bobtimus-baru to master August 14, 2021 06:29
Coin selection and estimate transaction size has been moved to baru
@mergify mergify bot merged commit 95d0657 into master Aug 17, 2021
@mergify mergify bot deleted the extract-wallet branch August 17, 2021 00:48
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.

4 participants