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

[custom channels]: add decimal display to channel funding blob #1245

Merged
merged 8 commits into from
Dec 9, 2024

Conversation

guggero
Copy link
Member

@guggero guggero commented Dec 6, 2024

Fixes #1067.

Adds the decimal display value to the channel funding blob, so it can be rendered for lncli listchannels and lncli pendingchannels.
To make the code in the funding manager as nice as possible, we first do some refactor.

To avoid a circular package dependency between the address and proof
package, we move the GenChallengeNUMS function into the assets package.
That function is the only reference the proof package has to the address
package, so the move removes that dependency.
To resolve another circular dependency issue in an upcoming commit, this
time between the address and taprpc package, we move two more functions
to another place where they fit better.
In general, any RPC package should _not_ have a dependency into other
packages, so this was wrong in the first place.
We'll want to re-use the DecDisplayOption method in other places, so we
move it from the RPC server to the meta reveal struct itself.
To make it possible that we can sync an asset when querying for the
asset meta information, we move the two meta related queries to the
address book. That way we can use the sync enabled asset meta fetch in a
later commit in the channel funding controller.
With the address book now implementing that method, we can add it to our
AssetSyncer interface so we can query asset meta information during
channel funding.
In this commit, we check for input asset uniqueness for parcels in the
PreBroadcast state, before being converted to a Transfer. This prevents
invalid transfers from being published and logged via RPC.
@guggero guggero force-pushed the decimal-display-funding-blob branch from 85e51bf to 565ee3c Compare December 6, 2024 14:30
@coveralls
Copy link

coveralls commented Dec 6, 2024

Pull Request Test Coverage Report for Build 12200570488

Details

  • 57 of 241 (23.65%) changed or added relevant lines in 12 files are covered.
  • 19 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.06%) to 40.71%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapchannelmsg/custom_channel_data.go 0 1 0.0%
tapfreighter/parcel.go 0 4 0.0%
tapsend/send.go 13 18 72.22%
rpcserver.go 0 7 0.0%
proof/meta.go 0 16 0.0%
address/address.go 0 24 0.0%
address/book.go 2 34 5.88%
tapdb/addrs.go 0 44 0.0%
tapchannel/aux_funding_controller.go 0 51 0.0%
Files with Coverage Reduction New Missed Lines %
tapdb/assets_store.go 1 65.12%
rpcserver.go 1 0.0%
asset/asset.go 2 81.43%
tapgarden/caretaker.go 4 68.87%
tapchannel/aux_leaf_signer.go 5 43.08%
tapdb/multiverse.go 6 68.21%
Totals Coverage Status
Change from base Build 12195811359: -0.06%
Covered Lines: 25821
Relevant Lines: 63427

💛 - Coveralls

Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

LGTM + tACK 💯

{
  "assets": [
    {
      "asset_utxo": {
        "version": 1,
        "asset_genesis": {
          "genesis_point": "d658833784d47ef01150b4f0cf48d7280eac7519b7c2af99d31457cce103a75e:0",
          "name": "itest-asset-cents",
          "meta_hash": "7e54bc90a6b94efa013ad03f07d2850e6d7a3d21135038c88e2436fa4ef9bc48",
          "asset_id": "644697197164440f0236d85c0d60515922e1ce949d4083caa98d30f9eb155baa"
        },
        "amount": 400000,
        "script_key": "0250aaeb166f4234650d84a2d8a130987aeaf6950206e0905401ee74ff3f8d18e6",
        "decimal_display": 4
      },
      "capacity": 400000,
      "local_balance": 400000,
      "remote_balance": 0
    }
  ]
}

Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@guggero guggero merged commit 4a15eb2 into main Dec 9, 2024
18 checks passed
@guggero guggero deleted the decimal-display-funding-blob branch December 9, 2024 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[feature]: lncli listchannels should (probably) show the decimal_display value of an asset
5 participants