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

Address send utils #67

Merged
merged 1 commit into from
Aug 9, 2022
Merged

Address send utils #67

merged 1 commit into from
Aug 9, 2022

Conversation

jharveyb
Copy link
Collaborator

Requires #56, split from #43.

Includes first step of sending, validating the sender input Taro tree against a receiver Taro address. I imagine most errors on sending should get caught at this step.

Would like feedback on how to test P2TR script creation and signVirtualKeySpend, if possible or necessary.

@dstadulis
Copy link
Collaborator

Kinda like coinselection and validation

@guggero guggero self-requested a review July 27, 2022 19:01
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.

Did a quick first round, still trying to figure out some parts of the code that I'm unfamiliar with, so will do another one soon.

address/params.go Show resolved Hide resolved
address/params.go Outdated Show resolved Hide resolved
address/address.go Outdated Show resolved Hide resolved
address/address.go Show resolved Hide resolved
address/address.go Show resolved Hide resolved
address/address_test.go Show resolved Hide resolved
address/address_test.go Show resolved Hide resolved
address/address_test.go Outdated Show resolved Hide resolved
address/address_test.go Outdated Show resolved Hide resolved
address/address_test.go 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.

The linter complains about an unused function.
Otherwise LGTM, pending @bhandras' comments.

address/address.go Outdated Show resolved Hide resolved
if err != nil {
return nil, err
}
taprootPrivKey := txscript.TweakTaprootPrivKey(&privKey, nil)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the correct way to tweak keys for Taproot when there is no script path spend / only key spend; I should double check against existing Taproot / wallet code though.

Yep, this is correct. Though I assume that BIP 86 would be used when just a keypath spend is needed...

Right now in #73 I just use a normal key for the script key, but I think it makes sense to make that a bip 86 key for now when we don't need any other fancy redemption

@@ -127,6 +145,42 @@ func New(id asset.ID, familyKey *btcec.PublicKey, scriptKey btcec.PublicKey,
return &payload, nil
}

// isValidInput verifies that the Taro commitment of the input contains an
// asset that could be spent to the given Taro address.
func isValidInput(input *commitment.TaroCommitment, address AddressTaro,
Copy link
Member

Choose a reason for hiding this comment

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

I'm aware this was broken out from the larger PR, but I don't think it belongs in the base address package. I'd expect it to live where ever the batched sender ultimately lives

Script()
}

// signVirtualKeySpend generates a signature over a Taro virtual transaction,
Copy link
Member

Choose a reason for hiding this comment

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

Same goes for this function (re ideally living in another package). It should also ahve some test tacked laong w/ it, thouhg I think this was sort of covered in the vm package to test normal validation.

We could try to make an optimistic taroscript package to house these send/receive related utilities for now?

@bhandras
Copy link
Member

bhandras commented Aug 8, 2022

Ready for next round @jharveyb ? Please re-request once it's ready.

@jharveyb
Copy link
Collaborator Author

jharveyb commented Aug 8, 2022

Ready for next round @jharveyb ? Please re-request once it's ready.

Rebased and addressed feedback + removed signVirtualKeySpend() from this PR - Was planning to move some functions to a new taroscript package in a future PR.

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 🌮

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 🌻

@Roasbeef Roasbeef merged commit ff469d1 into main Aug 9, 2022
@guggero guggero deleted the address-send-utils branch August 9, 2022 07:27
dstadulis pushed a commit that referenced this pull request May 16, 2023
itest: add more test cases that use grouped assets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants