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

[1/?] Static Loop-In Address - Create #642

Merged
merged 11 commits into from
Mar 4, 2024

Conversation

hieblmi
Copy link
Collaborator

@hieblmi hieblmi commented Sep 28, 2023

This draft PR implements static address generation for loop-ins.
It includes new command line options for loop, namely ~$ loop in static new a.k.a ~$ loop in s n.

Missing features to consider this PR complete are:

  • unit tests / mocks

@hieblmi hieblmi self-assigned this Sep 28, 2023
@hieblmi hieblmi marked this pull request as draft September 28, 2023 12:42
@levmi levmi added the loop in label Sep 28, 2023
@hieblmi hieblmi mentioned this pull request Oct 12, 2023
@hieblmi hieblmi force-pushed the static-addr-1 branch 2 times, most recently from 9818ae1 to d28f6b7 Compare October 13, 2023 09:18
cmd/loop/staticaddr.go Outdated Show resolved Hide resolved
staticaddr/actions.go Outdated Show resolved Hide resolved
@hieblmi hieblmi changed the title WIP: [1/?] Static Loop-In Address [1/?] Static Loop-In Address Oct 18, 2023
@hieblmi hieblmi force-pushed the static-addr-1 branch 2 times, most recently from a6ed4ed to 85a284b Compare October 18, 2023 11:39
cmd/loop/staticaddr.go Outdated Show resolved Hide resolved
@hieblmi hieblmi force-pushed the static-addr-1 branch 2 times, most recently from 0bfc4c4 to 2bfc438 Compare October 25, 2023 10:56
@hieblmi hieblmi changed the title [1/?] Static Loop-In Address [1/?] Static Loop-In Address - Create Oct 25, 2023
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Great work! Did just a very light pass to see where the PR is.

loopdb/sqlc/migrations/000003_static_address.up.sql Outdated Show resolved Hide resolved
loopdb/sqlc/migrations/000003_static_address.up.sql Outdated Show resolved Hide resolved
loopdb/sqlc/migrations/000003_static_address.down.sql Outdated Show resolved Hide resolved
fsm/fsm.go Outdated Show resolved Hide resolved
staticaddr/actions.go Outdated Show resolved Hide resolved
staticaddr/address_interface.go Outdated Show resolved Hide resolved
staticaddr/manager.go Outdated Show resolved Hide resolved
staticaddr/manager.go Outdated Show resolved Hide resolved
swapserverrpc/server.proto Outdated Show resolved Hide resolved
staticaddr/script/script.go Outdated Show resolved Hide resolved
@hieblmi hieblmi force-pushed the static-addr-1 branch 3 times, most recently from d62812f to 4081190 Compare November 8, 2023 14:28
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM (just needs clean rebase to a fresh master) 🎉 🎉

@lightninglabs-deploy
Copy link

@sputn1ck: review reminder
@GeorgeTsagk: review reminder

Copy link
Member

@sputn1ck sputn1ck left a comment

Choose a reason for hiding this comment

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

Very close, tested this out and works great.

Just a small db optmization we could do and a ux thing IMO

loopdb/sqlc/migrations/000007_static_address.up.sql Outdated Show resolved Hide resolved

message ServerAddressParameters {
// A unique identifier for static address parameters assigned by the server.
bytes id = 1;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think a unique identifier is needed. Both server and client can just use the pkscript

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought this might be handy once we enable multple static addresses per client?

Copy link
Member

Choose a reason for hiding this comment

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

They will also have a unique scriptkey, as both parties should not reuse keys.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed this ID

swapserverrpc/server.proto Outdated Show resolved Hide resolved
looprpc/staticaddressclient.pb.json.go Show resolved Hide resolved
// NewAddress starts a new address creation flow.
func (m *Manager) NewAddress(ctx context.Context) (*AddressParameters,
error) {

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't it make sense to fetch an existing address first and return this? As we're not supporting multiple addresses we can have this first check here. I think on the cli it would make sense to just return the current address if you run loopcli s new

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@hieblmi hieblmi force-pushed the static-addr-1 branch 4 times, most recently from 2723522 to 4570bfd Compare February 26, 2024 13:36
@hieblmi hieblmi requested a review from sputn1ck February 26, 2024 13:37
Copy link
Member

@sputn1ck sputn1ck left a comment

Choose a reason for hiding this comment

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

LGTM!!!! Great job and happy start to the merge journey

@hieblmi hieblmi merged commit d5cb601 into lightninglabs:static-addr-staging Mar 4, 2024
4 checks passed
@hieblmi hieblmi deleted the static-addr-1 branch March 4, 2024 17:32
@hieblmi hieblmi mentioned this pull request Jun 28, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants