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(tapd): Add support for Taproot Assets Protocol nodes #641

Merged
merged 52 commits into from
May 16, 2023

Conversation

jamaljsr
Copy link
Owner

@jamaljsr jamaljsr commented Dec 29, 2022

Closes #601

Description

This PR implements the necessary changes required to support Taro nodes (tarod) in Polar.

⚠️ Draft Status ⚠️

Full support for Taro is not complete. It requires implementing additional features and resolving the known issues below:

Features to implement

  • Publish Docker images for LND and tarod master branches
  • Add draggable Taro node to the sidebar
  • Allow access to tarod logs and Terminal
  • Display asset balances and details in the sidebar
  • mint an asset
  • create an address to receive an asset
  • send an asset

Known issues

  • update docker ports for tarod nodes when the network starts
  • fix error when removing a Taro node from the network
  • drag a tarod->lnd link to change which LND node tarod connects to
  • display sidebar info for tarod->lnd links
  • when adding a Taro node, show an error if there are no LND nodes available running master
  • update tarod paths when importing a network
  • add all of the necessary unit tests
  • change LND & tarod versions to be based on a date (ex: 2022.12.28-master)
  • clear RPC cache when a node/network is started to handle cert/macaroon recreations (fixed in fix(network): clear RPC cache when nodes are stopped #698)

Steps to test

This PR makes use of the @hodlone/taro-api NPM package, but requires some minor changes to get it working in Polar. If you want to run this branch locally, you'll need to follow the instructions in my fork jamaljsr/taro-api.

  1. Create a new network with only a Bitcoin Core node. Do not add any LND nodes
  2. Drag two LND master branch nodes (ex: v2022.12.28-master) onto the canvas
  3. Drag two Taro nodes onto the canvas
  4. Start the network
  5. Launch the Terminal for both Taro nodes
  6. Use tarocli in the terminals to mint an asset and send/receive them between the two nodes

Screenshots

image

Expand to view more screenshots

image

image

@amovfx
Copy link
Contributor

amovfx commented Dec 29, 2022

I will tackle minting an asset.

@amovfx
Copy link
Contributor

amovfx commented Jan 1, 2023

So I have a working MintAssetModal and have run into a some issues where I need expanded functionality when it comes to testing.

The create network function is lacking the ability to create Taro needs which it looks like the testing requires. This is in network.ts and the function createNetwork.

I'm going to start another branch to address this, it involves allowing the network creator to make taro nodes.

Is this a good direction to go or is it more beneficial for me to build the functionality out in your list above?

@jamaljsr
Copy link
Owner Author

jamaljsr commented Jan 1, 2023

I intentionally didn't update the createNetwork function to accept the number of Taro nodes because tarod requires LND master which I don't want to make default for all networks. We could make it detect if Taro was chosen then use the correct LND but then we'd need to hard code versions and it would just be a temporary bandaid that would need to be removed once a compatible tagged version of LND is released.

For testing, a simple solution is to just add a Taro node after the initial network is created.

@amovfx
Copy link
Contributor

amovfx commented Jan 5, 2023

Hey @jamaljsr,
I'm blocking out send asset right now and was wondering if you have input on the workflow.
The receiver has to import the assets genesis_bootstrap_info. I was planning to do this implicitly on send action if the receiver doesn't hold the address. On send, the recipient would import the genesis_bootstrap_info behind the scenes, then the asset would be sent.

Do you think this is a bad idea, and that importing genesis_bootstrap_info should be an explicit step?
I was also thinking of displaying a warning with a fix button. in the Send asset Modal when an asset is selected that the recipient doesn't have.
"Warning, bob does not have this asset, import genesis_bootstrap_info: "
Upon pressing fix, bob would import the assets genesis_bootstrap_info and the send button would become enabled.

@jamaljsr
Copy link
Owner Author

jamaljsr commented Jan 8, 2023

Hey @amovfx, I think importing of the genesis_bootstrap_info should be explicit since it's required to receive an asset for now. I believe that once Taro implements universes, this will no longer be required, then we can change the flow. Until then, anyone building an app on Taro will need to provide this info when generating an address, so Polar should prompt for it. I know it seems like an extra step when sending from/to Polar nodes, but always try to keep in mind that people will use Polar with external apps, so we don't want to automate too much. This is very similar to send/receive on Lightning with invoices. Polar requires the user to copy/paste the invoice even though this step could be skipped.

@amovfx
Copy link
Contributor

amovfx commented Jan 8, 2023

@jamaljsr
Awesome, this is the exact guidance I was looking for. I'll add the ability for an explicit step today.

@amovfx
Copy link
Contributor

amovfx commented Jan 11, 2023

@jamaljsr
On the branch send-taro-asset, I have a modal blocked in for creating a taro asset. Wouldn't mind getting feedback on the workflow.

I'm tracking down a bug where genesisBootstrapInfo value is being rest when the amount is changed, but the overall workflow is in place and ready for a critique.

@jamaljsr
Copy link
Owner Author

@amovfx Please create a PR if you would like me to review the code for that. Just be sure to set the target branch to be your mint assets PR if your new commits are on top of those.

@jamaljsr
Copy link
Owner Author

jamaljsr commented Feb 7, 2023

@amovfx I updated this branch with unit tests for all the new Taro features. It is back at 100% coverage now. Please rebase your #656 branch on the latest here. You can also run yarn ci on your machine before pushing to github to confirm the Github checks will succeed.

@amovfx
Copy link
Contributor

amovfx commented Feb 7, 2023

Okay Thanks for the heads up

@amovfx
Copy link
Contributor

amovfx commented Feb 7, 2023

K I've been struggling with resets/rebases, It looks like it is working, I think it would need your eyes to go over this.
Tests and listing are clear on my end.

@amovfx
Copy link
Contributor

amovfx commented Feb 8, 2023

I'm currently working on "drag a tarod->lnd link to change which LND node tarod connects to", let me know if you think an adjustment of priorities would be best.

@jamaljsr
Copy link
Owner Author

jamaljsr commented Feb 8, 2023

I'm currently working on "drag a tarod->lnd link to change which LND node tarod connects to", let me know if you think an adjustment of priorities would be best.

Cool. that sounds good. I don't think there's much left to do. We're getting close to done on this.

@jamaljsr
Copy link
Owner Author

jamaljsr commented Feb 24, 2023

While testing, I've run into some minor improvements that can be made. These should be resolved before merging this PR.

Generate Address

  • when generating an address, don't show the name of nodes with no assets in the dropdown

Send Asset

  • send asset from alice to bob then from bob to carol requires manually mining 1 extra block. In the UI, no error is displayed. In the CLI, it says "[tarocli] unable to send assets: rpc error: code = Unknown desc = unable to fund psbt: unable to fund psbt: rpc error: code = Unknown desc = wallet couldn't fund PSBT: error creating funding TX: insufficient funds available to construct transaction"
  • missing i18n key cmps.designer.taro.actions.SendAssetModal.submitError
  • Send button doesn't disable or show loader when clicked

Mint Asset

  • interface NewAddressModel in modals.ts has unnecessary fields (amount, genesisBootstrapInfo, address)
  • "Successfully minted asset: 'AAA' with batchkey:" is missing the batch key. We should just remove "with batchkey:" from the message since the user can't really do anything with it

General

  • On the sidebar, display a "This node does not have any assets." message in the Assets section when there aren't any assets in the list
  • I don't think we need to fetch and store balances in addition to assets. We could just sum up the asset amounts when a total balance is needed. Maybe with a computed field on the taro store.
  • With 3 LND nodes, when dragging taro nodes onto the canvas, they are added in the incorrect order of alice-taro -> carol-taro -> bob-taro.

@amovfx Feel free to tackle any of these if you'd like. Please just create a separate PR per group to make review a bit easier.

@jamaljsr jamaljsr marked this pull request as ready for review February 25, 2023 03:30
@amovfx
Copy link
Contributor

amovfx commented Feb 25, 2023

Will do! Thanks for the feedback!

@amovfx
Copy link
Contributor

amovfx commented Feb 26, 2023

I'm having trouble conceptualizing. I have the same feeling, it seems like the only different thing you get from the taro asset is the total supply.

 I don't think we need to fetch and store balances in addition to assets. We could just sum up the asset amounts when a total balance is needed. Maybe with a computed field on the taro store.

I hit the other notes. I'm also wondering if we are at the point where we want to add taro nodes to the network create pane?

@jamaljsr
Copy link
Owner Author

I'm having trouble conceptualizing. I have the same feeling, it seems like the only different thing you get from the taro asset is the total supply.

I'd have to do a deeper analysis of the RPC responses but I know they are virtually identical. I'm just not sure if there's any unique info in the listBalances response that's not in listAssets. One thing I did notice is that listAssets can return multiple records per asset. One record for each UTXO if the node receives the same asset multiple times. So I believe this is the one we should keep because it has more details.

I hit the other notes. I'm also wondering if we are at the point where we want to add taro nodes to the network create pane?

I don't think we should do that until the official mainnet version is released. If you want to work on it, you can. But you are likely going to be doing a bunch of rebasing.

@amovfx
Copy link
Contributor

amovfx commented Feb 27, 2023

I'd have to do a deeper analysis of the RPC responses but I know they are virtually identical. I'm just not sure if there's any unique info in the listBalances response that's not in listAssets. One thing I did notice is that listAssets can return multiple records per asset. One record for each UTXO if the node receives the same asset multiple times. So I believe this is the one we should keep because it has more details.

Sounds good, Ill poke around too when I get some time.

I don't think we should do that until the official mainnet version is released. If you want to work on it, you can. But you are likely going to be doing a bunch of rebasing.

Yea that sounds good, I know a change to the meta data is happening where it will just be a hash. Probably other improvements are coming as well. I'll hold off.

@amovfx
Copy link
Contributor

amovfx commented Mar 6, 2023

I found the rpccahce stuff, and I'm poking around it right now, is it something you still want to tackle?

@jamaljsr
Copy link
Owner Author

Rebased on master

Andrew Oseen and others added 18 commits May 16, 2023 12:10
Proper casing on the usePrefixedTranslation
Added tarp send asset functionality.
Fixed test to check for label to deposit
enough funds for mint asset modal
NewAddressResponse renamed to AddressResponse
Hit notes and made sure tests hit coverage.
Cleaned up the tests
linting errors, missing parameter, and testing conventions
Send asset is at the top now.
Metadata needed to be a base64 string.

Co-authored-by: Andrew Oseen <[email protected]>
-Increase the lightning wallet balance check by 2x.
-Send button now has loading state.
-Added missing error state

Co-authored-by: Andrew Oseen <[email protected]>
-taro nodes connect in order when dropped into the graph
-Taro info asset draw title says 'No Assets' if there are no assets

---------

Co-authored-by: Andrew Oseen <[email protected]>
@jamaljsr jamaljsr changed the title [WIP] feat(taro): Add support for Taro nodes feat(tapd): Add support for Taproot Assets Protocol nodes May 16, 2023
@jamaljsr jamaljsr merged commit 55921d0 into master May 16, 2023
@jamaljsr jamaljsr deleted the feat/add-taro branch May 16, 2023 18:24
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.

Feature Request: Taro daemon
2 participants