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

add Signer interface for Taro virtual transactions #116

Merged
merged 1 commit into from
Sep 14, 2022
Merged

Conversation

jharveyb
Copy link
Collaborator

@jharveyb jharveyb commented Sep 12, 2022

Spun off from send state machine, builds on #115, only last commit is new.

In this PR, we introduce a Signer interface responsible for signing over Taro virtual TXs. This is needed to connect the sending state machine to a backing lnd client, and not require direct access to private keys.

I've added an equality check between the mock Signer and the old 'direct' signing method, and the existing unit tests pass, so AFAICT this interface + mock logic works. Requesting feedback to make sure that isn't a fluke + general improvements.

Outstanding tasks before pulling out of draft:

  • Update VM tests to sign with Signer interface
  • Fully remove 'direct' signing method
  • Remove equality check in CompleteAssetSpend
  • Clarify constraints and limitations (BIP86 only , 1-in 1-out only, etc.) where needed

@Roasbeef
Copy link
Member

Update VM tests to sign with Signer interface

I don't think they actually need to, given the unit tests can just use a canned private key signer (or maybe that's what you mean?). Similar to this: https://github.com/lightninglabs/taro/blob/main/asset/asset.go#L330-L334

Remove equality check in CompleteAssetSpend

Which check?

Fully remove 'direct' signing method

I think we'd keep this one to make a mock signer that just uses the raw private key for unit tests.

taroscript/interface.go Outdated Show resolved Hide resolved
taroscript/mock.go Show resolved Hide resolved
taroscript/send.go Outdated Show resolved Hide resolved
taroscript/tx.go Show resolved Hide resolved
virtual_tx_signer.go Outdated Show resolved Hide resolved
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Needs a rebase!
Interface and mock look good, though I think some things can be simplified a bit.

taroscript/mock.go Outdated Show resolved Hide resolved
taroscript/tx.go Show resolved Hide resolved
taroscript/mock.go Outdated Show resolved Hide resolved
taroscript/tx.go Outdated Show resolved Hide resolved
taroscript/tx.go Outdated Show resolved Hide resolved
taroscript/tx.go Outdated Show resolved Hide resolved
@jharveyb jharveyb marked this pull request as ready for review September 14, 2022 00:54
@jharveyb
Copy link
Collaborator Author

Rebased, updated to address all feedback, also added more plumbing for merge / multi-input transfers.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🥟

// descriptor and TX.

// NOTE: We currently assume the signature requested is for the
// BIP 86 spending type.
Copy link
Member

Choose a reason for hiding this comment

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

Seems fine for the scope of our current milestone, but isn't this mainly dependent on the SIgnMethod in the sign desc?

So then it's up to the caller to also provide the witness script (for the leaf being revealed/spent).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the interface itself doesn't prevent non-BIP86 use. This assumption is really enforced earlier on in SignTaprootKeySpend, there isn't currently a way to provide a witness script at all.

return nil, err
}

validatedAsset.PrevWitnesses[idx].TxWitness = *newWitness
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

Saw a few nits but am going to address those in my proof append PR where I can use some of the stuff declared in this PR.

@guggero guggero merged commit b0310cd into main Sep 14, 2022
@guggero guggero deleted the signer-interface branch September 14, 2022 11:09
@dstadulis dstadulis changed the title [WIP] Signer interface add Signer interface for Taro virtual transactions Sep 14, 2022
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.

None yet

3 participants