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

feat: runes balances #5224

Merged
merged 1 commit into from
Apr 12, 2024
Merged

feat: runes balances #5224

merged 1 commit into from
Apr 12, 2024

Conversation

fbwoolf
Copy link
Contributor

@fbwoolf fbwoolf commented Apr 11, 2024

Try out Leather build cf3f7ddExtension build, Test report, Storybook, Chromatic

This PR adds Runes balances to our assets list. This implementation only shows Runes for the first taproot index. We can expand it later (or now) if needed.

@kyranjamie hoping for your feedback here bc I'm trying to consider api response data vs what we need for our types later. I followed a similar pattern that was used for BRC-20 tokens where we select the response and add data to it, like the balance conversion to our Money type seen here, but I do think we need a better way to organize these models/types ...and I don't think we did a great job setting up Bitcoin/Stacks fungible tokens. 🤔 If we want to continue with that pattern, we should really be creating Bitcoin fungible token types in our models, which we haven't been doing. This bitcoin-client file is getting kinda messy imo. I'm not sure about doing a big refactor with Runes bc we need to get this out, but I do think this needs to be revisited so we can scale it better?

Screenshot 2024-04-11 at 4 01 44 PM

@fbwoolf fbwoolf force-pushed the feat/runes-balances branch from 1f2bdba to 3bd4453 Compare April 11, 2024 21:19
@fbwoolf fbwoolf linked an issue Apr 11, 2024 that may be closed by this pull request
Copy link
Collaborator

@kyranjamie kyranjamie 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 a really tidy first implementation Fara, great job.

RE how we structure Runes, I imagine we might have a general Rune/RuneEtching model in @leather/models—something very similar to, but not necessarily exactly the same as, the response from the API.

As this is super new though, I don't think it's important to get the modelling right immediately. Doesn't hurt to just use API types for now, and see how things develop. We'll better know how to model it later.

Agree BitcoinClient is a mess. I'm not huge on the class-based approach here, which I think came from copying the auto-generated blockchain api early on. A flat suite of functions taking a simple network config would be sufficient.

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Apr 12, 2024

I am going to remove all the hardcoded testnet code here and get this merged into dev so it is ready. We will get a 404 on main for the api call rn, but that should change next week. Added to remote config.

@fbwoolf fbwoolf force-pushed the feat/runes-balances branch 2 times, most recently from 845fa88 to 3a173b8 Compare April 12, 2024 14:28
@fbwoolf fbwoolf force-pushed the feat/runes-balances branch from 3a173b8 to cf3f7dd Compare April 12, 2024 14:30
@fbwoolf fbwoolf added this pull request to the merge queue Apr 12, 2024
Merged via the queue into dev with commit 4e1de2c Apr 12, 2024
28 checks passed
@fbwoolf fbwoolf deleted the feat/runes-balances branch April 12, 2024 14:43
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.

[Runes] Display rune balances
3 participants