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.

Sign up for GitHub

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

Jump to bottom

Configure asset metadata #659

Merged
merged 4 commits into from
Jan 24, 2025
Merged

Configure asset metadata #659

merged 4 commits into from
Jan 24, 2025

Conversation

dangeross
Copy link
Collaborator

Builds on PR #645

This PR:

  • adds default asset metadata for Liquid Bitcoin and Tether USD
  • adds asset metadata if available into the payment details and get_info response
  • parses the BIP21 URI and adjusts the amount_sats precision to the asset metadata
  • adds to the Config the option to add asset metadata for additional supported assets

Depends on breez/breez-sdk-greenlight#1157

@dangeross dangeross mentioned this pull request Jan 14, 2025
Merged
@dangeross dangeross force-pushed the savage-asset-metadata branch from e6a24e3 to 2e0d015 Compare January 14, 2025 15:58
@roeierez
Copy link
Member

I have to admit at first I thought we should provide a larger list of assets but this minimal approach is more appealing now.
Having a default of a minimal list of assets and let the user define more may work better as a start.
@kingonly what do you think? Can we start with lbtc and usdt as built in assets?

@kingonly
Copy link
Member

Yes

@dangeross dangeross marked this pull request as ready for review January 15, 2025 13:00
roeierez
roeierez reviewed Jan 15, 2025
View reviewed changes
lib/core/src/persist/asset_metadata.rs
use super::{AssetMetadata, Persister};

impl Persister {
pub(crate) fn replace_asset_metadata(
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate what is the advantage of persisting the configurable assets?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For rendering the payment list, it seems simpler to add them to the SQL join

danielgranhao
danielgranhao reviewed Jan 17, 2025
View reviewed changes
Copy link
Contributor

@danielgranhao danielgranhao left a comment

Choose a reason for hiding this comment

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

Still reviewing, but will have to finish later, so here's what I got for now :)

lib/bindings/src/breez_sdk_liquid.udl Outdated Show resolved Hide resolved
lib/bindings/src/breez_sdk_liquid.udl Show resolved Hide resolved
lib/core/src/model.rs Outdated Show resolved Hide resolved
lib/core/src/model.rs Outdated Show resolved Hide resolved
@dangeross dangeross force-pushed the savage-multi-assets branch 2 times, most recently from 36f9697 to 289d61f Compare January 17, 2025 19:54
@dangeross dangeross force-pushed the savage-asset-metadata branch 2 times, most recently from dbc649c to e01ea48 Compare January 17, 2025 21:13
@dangeross dangeross force-pushed the savage-multi-assets branch from 289d61f to 4dfef3b Compare January 20, 2025 09:15
@dangeross dangeross force-pushed the savage-asset-metadata branch from e01ea48 to 2e7501a Compare January 20, 2025 09:33
@dangeross dangeross mentioned this pull request Jan 21, 2025
Merged
@roeierez roeierez added this to the v0.7.0 milestone Jan 22, 2025
hydra-yse
hydra-yse approved these changes Jan 23, 2025
View reviewed changes
Copy link
Member

@hydra-yse hydra-yse left a comment

Choose a reason for hiding this comment

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

LGTM

@dangeross dangeross force-pushed the savage-asset-metadata branch from eab5359 to 591f5e3 Compare January 24, 2025 08:31
@dangeross dangeross force-pushed the savage-multi-assets branch from b75822a to b066c8b Compare January 24, 2025 08:48
@dangeross dangeross force-pushed the savage-asset-metadata branch from 591f5e3 to b92d1ef Compare January 24, 2025 08:51
@dangeross dangeross force-pushed the savage-multi-assets branch from b066c8b to 8707492 Compare January 24, 2025 10:44
@dangeross dangeross force-pushed the savage-asset-metadata branch 2 times, most recently from 74ab74f to 0cbb386 Compare January 24, 2025 11:13
Base automatically changed from savage-multi-assets to main January 24, 2025 15:55
@dangeross dangeross force-pushed the savage-asset-metadata branch from 0cbb386 to b7db112 Compare January 24, 2025 15:57
@dangeross dangeross merged commit 652c238 into main Jan 24, 2025
9 checks passed
@dangeross dangeross deleted the savage-asset-metadata branch January 24, 2025 15:57
@dangeross dangeross mentioned this pull request Jan 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hydra-yse hydra-yse hydra-yse approved these changes

@danielgranhao danielgranhao danielgranhao approved these changes

@roeierez roeierez roeierez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.7.0
Development

Successfully merging this pull request may close these issues.
5 participants
@dangeross @roeierez @kingonly @danielgranhao @hydra-yse