-
Notifications
You must be signed in to change notification settings - Fork 137
chore: readme tlc #1895
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
base: main
Are you sure you want to change the base?
chore: readme tlc #1895
Conversation
Updates the listings for the required lnd and Go versions, and makes a mild aesthetic pass over the text more generally.
Summary of ChangesHello @jtobin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on maintaining the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the required versions for lnd and Go in the README.md and improves its overall formatting and readability. The changes are mostly good, but I have a few suggestions to ensure the documentation remains accurate and complete. Specifically, I've pointed out a potential issue with the git clone command, the removal of some useful information about mainnet compatibility, and a section heading that could be more descriptive.
| ``` | ||
|
|
||
| Add multiple universes to your local federation to always stay up to date. You can also use the universe to query existing assets and their metadata. You may also configure your tapd instance to listen to incoming requests with `--rpclisten 0.0.0.0:10029` to run your own universe. | ||
| Add multiple universes to your local federation to always stay up | ||
| to date. You can also use the universe to query existing assets and | ||
| their metadata. You may also configure your tapd instance to listen | ||
| to incoming requests with `--rpclisten 0.0.0.0:10029` to run your own | ||
| universe. | ||
|
|
||
| ```shell | ||
| tapcli universe federation add --universe_host testnet.universe.lightning.finance | ||
| tapcli universe roots | ||
| $ tapcli universe federation add \ | ||
| --universe_host testnet.universe.lightning.finance | ||
| $ tapcli universe roots | ||
| ``` | ||
|
|
||
| Once you have obtained the necessary proofs and asset IDs, you can generate a Taproot Asset address for a specific asset and amount. | ||
| Once you have obtained the necessary proofs and asset IDs, you can | ||
| generate a Taproot Asset address for a specific asset and amount. | ||
|
|
||
| ```shell | ||
| tapcli addrs new --asset_id bab08407[...]129bf6d0 --amt 21 | ||
| $ tapcli addrs new --asset_id bab08407[...]129bf6d0 --amt 21 | ||
| ``` | ||
|
|
||
| The sender can now fulfill the request by initiating the transfer. | ||
| The sender can now fulfill the request by initiating the transfer: | ||
|
|
||
| ```shell | ||
| tapcli assets send --addr taptb1q[...]tywpre3a | ||
| $ tapcli assets send --addr taptb1q[...]tywpre3a | ||
| ``` | ||
| ## Development | ||
|
|
||
| ### API | ||
|
|
||
| Taproot Assets exposes a GRPC (port 10029) and a REST (port 8089) API. Connections are encrypted with TLS and authenticated using macaroons. [The API is documented here](https://lightning.engineering/api-docs/api/taproot-assets/). Further guides [can be found here](https://docs.lightning.engineering/lightning-network-tools/taproot-assets). | ||
| Taproot Assets exposes a gRPC (port 10029) and a REST | ||
| (port 8089) API. Connections are encrypted with TLS and | ||
| authenticated using macaroons. The API is documented | ||
| [here](https://lightning.engineering/api-docs/api/taproot-assets/), and | ||
| further guides can be found | ||
| [here](https://docs.lightning.engineering/lightning-network-tools/taproot-assets). | ||
|
|
||
| ### Mainnet | ||
|
|
||
| Starting with the release of version `v0.3.0` the daemon does support the | ||
| Bitcoin `mainnet`. | ||
| `tapd` has supported mainnet since version `v0.3.0`. | ||
|
|
||
| **IMPORTANT NOTE**: To avoid loss of funds, it's imperative that you read the | ||
| [Operational Safety Guidelines](docs/safety.md) before using `tapd` on | ||
| `mainnet`! | ||
| mainnet! | ||
|
|
||
| The daemon is still in `alpha` state, which means there can still be bugs and | ||
| not all desired data safety and backup mechanisms have been implemented yet. | ||
| Releasing on `mainnet` mainly signals that there will be no breaking changes in | ||
| the future and that assets minted with `v0.3.0` will be compatible with later | ||
| versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph explaining the implications of releasing on mainnet (no breaking changes, forward compatibility) seems very useful for users. This information provides important context about asset compatibility across versions and gives confidence to users minting assets. Was its removal intentional? If not, I'd recommend re-adding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed that sentence as it seemed redundant to me. "We support mainnet" seems sufficient.
| **WITHOUT BACKUP BEFORE DELETION, FUNDS ARE DESTROYED**. | ||
|
|
||
| ## RFQ, asset decimal display, unit precision and price oracle | ||
| ## RFQ and Price Oracle System |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new heading RFQ and Price Oracle System is less descriptive than the original. The linked document docs/rfq-and-decimal-display.md also covers "asset decimal display" and "unit precision" in detail. To avoid confusion and accurately reflect the content, consider using a more comprehensive title.
| ## RFQ and Price Oracle System | |
| ## RFQ, Asset Precision, and Price Oracle System |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could go either way, just wanted to shorten it a bit.
Pull Request Test Coverage Report for Build 19894659504Details
💛 - Coveralls |
Roasbeef
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🧩
Updates the listings for the required lnd and Go versions, and makes a mild aesthetic pass over the text more generally.