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

Version 0.0.123 #131

Merged
merged 6 commits into from
May 22, 2024
Merged

Version 0.0.123 #131

merged 6 commits into from
May 22, 2024

Conversation

arik-so
Copy link
Contributor

@arik-so arik-so commented May 21, 2024

No description provided.

@arik-so arik-so force-pushed the version-0.0.123 branch 2 times, most recently from 4be66a6 to e36f0ac Compare May 21, 2024 09:26
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Mostly looks good, two comments.

Also might need to make sure we don't need to account for #130.

out/Bindings.swift Show resolved Hide resolved
@@ -30,13 +30,13 @@ class RegtestChannelManagerPersister : Persister, ExtendedChannelManagerPersiste
let fastFeerate = 7500
let destinationScriptHardcoded: [UInt8] = [118,169,20,25,18,157,83,230,49,155,175,25,219,160,89,190,173,22,109,249,10,184,245,136,172]

guard let result = self.keysManager?.spendSpendableOutputs(descriptors: outputs, outputs: [], changeDestinationScript: destinationScriptHardcoded, feerateSatPer1000Weight: UInt32(fastFeerate), locktime: nil) else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use the OutputSweeper instead here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nobody's using RegtestChannelManagerPersister anymore, but I can address it in a followup PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used an OutputSpender instead of the Sweeper.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, should work. It would be great to test the OutputSweeper works as expected in bindings eventually though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, I'm confused. OutputSpender is a trait, so if you just init OutputSpender() it shouldn't do anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoa, you're right. Wtf's going on lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM once CI passes, if we're positive there's nothing to do w.r.t. #130.

@arik-so arik-so enabled auto-merge May 22, 2024 07:38
@arik-so arik-so merged commit 12f857c into lightningdevkit:main May 22, 2024
12 checks passed
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.

2 participants