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

persist payments info to disk #104

Merged
merged 1 commit into from
Jul 25, 2023
Merged

persist payments info to disk #104

merged 1 commit into from
Jul 25, 2023

Conversation

zoedberg
Copy link
Contributor

This PR should close #90.

I hope the way I implemented this meets the goals of the sample (simplicity and readibility). I was a bit unsure on some styling details, if there's something you would like to be implemented differently I'd be happy to change it.

@zoedberg zoedberg force-pushed the persist_payments branch from c3c0473 to b88e454 Compare May 11, 2023 09:20
@tnull
Copy link
Contributor

tnull commented May 19, 2023

Thanks for having a go at this! I believe this unfortunately now needs a rebase!

@zoedberg zoedberg force-pushed the persist_payments branch from b88e454 to 7814899 Compare May 19, 2023 13:36
@zoedberg
Copy link
Contributor Author

No problem, it was an easy rebase!

tnull
tnull reviewed May 20, 2023
View reviewed changes
Copy link
Contributor

@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.

Generally looks good, comments mostly concerning serialization.

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
TheBlueMatt
TheBlueMatt reviewed May 21, 2023
View reviewed changes
src/disk.rs Outdated Show resolved Hide resolved
@zoedberg zoedberg force-pushed the persist_payments branch from c8c446c to 5378d42 Compare May 22, 2023 11:50
tnull
tnull reviewed May 22, 2023
View reviewed changes
src/disk.rs Outdated Show resolved Hide resolved
@zoedberg zoedberg force-pushed the persist_payments branch from 5378d42 to 9e0b09b Compare May 22, 2023 12:12
TheBlueMatt
TheBlueMatt reviewed Jun 6, 2023
View reviewed changes
Copy link
Contributor

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in responding here, somehow this fell off my todo list.

src/cli.rs Outdated Show resolved Hide resolved
@zoedberg zoedberg force-pushed the persist_payments branch from 9e0b09b to 339ea63 Compare June 7, 2023 12:40
@zoedberg zoedberg requested review from tnull and TheBlueMatt July 4, 2023 10:05
@zoedberg
Copy link
Contributor Author

@TheBlueMatt could you please check if this can now be merged?

TheBlueMatt
TheBlueMatt reviewed Jul 14, 2023
View reviewed changes
Copy link
Contributor

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Grrrr, I am so sorry I missed this again somehow. It basically looks good to me with one missed update. When you push next, feel free to squash the fixup commits down into one commit.

src/cli.rs Outdated Show resolved Hide resolved
@zoedberg
Copy link
Contributor Author

Grrrr, I am so sorry I missed this again somehow. It basically looks good to me with one missed update. When you push next, feel free to squash the fixup commits down into one commit.

No worries and thank you for the thorough review. I've fixed and squashed the fixup commits

TheBlueMatt
TheBlueMatt reviewed Jul 24, 2023
View reviewed changes
Copy link
Contributor

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM! Instead of squashing to have one commit which does work and a second commit that fixes it, we should have only commits which stand on their own and don't have fixes to them in later commits. Because this is all kinda one big set of change you're welcome to squash it down to a single commit.

@zoedberg
Copy link
Contributor Author

@TheBlueMatt I've squashed the commits into a single one

@zoedberg
Copy link
Contributor Author

And also rebased since I've just noticed commit was not on top of current main

TheBlueMatt
TheBlueMatt approved these changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks!

@TheBlueMatt TheBlueMatt merged commit 139e065 into lightningdevkit:main Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@tnull tnull Awaiting requested review from tnull

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Persist payment info to disk
3 participants
@zoedberg @tnull @TheBlueMatt