Skip to content

Make ChainMonitor::get_claimable_balances take a slice of refs #1093

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

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

For the same reason as get_route, a slice of objects isn't
practical to map to bindings - the objects in the bindings space
are structs with a pointer and some additional metadata. Thus, to
create a slice of them, we'd need to take ownership of the objects
behind the pointer, place them into a slace, and then restore them
to the pointer.

This would be a lot of memory copying and marshalling, not to
mention wouldn't be thread-safe, which the same function otherwise
would be if we used a slice of references instead of a slice of
objects.

For the same reason as `get_route`, a slice of objects isn't
practical to map to bindings - the objects in the bindings space
are structs with a pointer and some additional metadata. Thus, to
create a slice of them, we'd need to take ownership of the objects
behind the pointer, place them into a slace, and then restore them
to the pointer.

This would be a lot of memory copying and marshalling, not to
mention wouldn't be thread-safe, which the same function otherwise
would be if we used a slice of references instead of a slice of
objects.
@TheBlueMatt TheBlueMatt added this to the 0.0.101 milestone Sep 22, 2021
@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #1093 (d718822) into main (aad1803) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1093   +/-   ##
=======================================
  Coverage   90.73%   90.73%           
=======================================
  Files          65       65           
  Lines       34315    34315           
=======================================
  Hits        31136    31136           
  Misses       3179     3179           
Impacted Files Coverage Δ
lightning/src/chain/chainmonitor.rs 95.91% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aad1803...d718822. Read the comment docs.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM, though note that Rust users will need an additional Vec allocation when passing the result of ChannelManager::list_usable_channels, IIUC.

@TheBlueMatt
Copy link
Collaborator Author

Yea, it’s not ideal. I think in the next version we’ll want to revert this and drop the ref on get_route, but I don’t have time to fix that for the bindings this release.

@TheBlueMatt TheBlueMatt merged commit e12d88d into lightningdevkit:main Sep 22, 2021
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.

3 participants