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

DRY changes and adding support for BIP-239 Extended Transaction Format #130

Merged
merged 14 commits into from Nov 16, 2022

Conversation

ordishs
Copy link
Collaborator

@ordishs ordishs commented Nov 10, 2022

All TX parsing functions now delegate to the tx.ReadFrom() function. All duplicate code has been removed.

Added support for parsing raw transactions that are using the BIP-239 Extended Transaction Format.

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2022

Codecov Report

Base: 83.86% // Head: 83.70% // Decreases project coverage by -0.16% ⚠️

Coverage data is based on head (548edc8) compared to base (34e82d8).
Patch coverage: 75.51% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
- Coverage   83.86%   83.70%   -0.17%     
==========================================
  Files          34       34              
  Lines        3670     3701      +31     
==========================================
+ Hits         3078     3098      +20     
- Misses        426      434       +8     
- Partials      166      169       +3     
Impacted Files Coverage Δ
bscript/address.go 85.96% <ø> (ø)
bscript/interpreter/debug/debugger.go 97.70% <ø> (ø)
bscript/interpreter/errs/error.go 50.00% <ø> (ø)
unlocker/simple.go 65.00% <ø> (ø)
bscript/interpreter/state.go 29.16% <50.00%> (ø)
input.go 79.59% <62.50%> (-1.49%) ⬇️
tx.go 77.29% <74.07%> (-1.23%) ⬇️
bscript/interpreter/operations.go 96.54% <100.00%> (ø)
bscript/script.go 65.25% <100.00%> (+2.02%) ⬆️
txinput.go 80.00% <100.00%> (-2.50%) ⬇️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@theflyingcodr
Copy link
Contributor

Thanks for this Simon, will get at a look it. A quick Google has failed me though, where can I find info on BIP-239

@ordishs
Copy link
Collaborator Author

ordishs commented Nov 11, 2022 via email

@theflyingcodr
Copy link
Contributor

I see, that explains why I can't find it, hard to review the PR in that case :)

One thing, is it likely that will be other Tx formats / extensions such as this, if so we should probably think of a nice way of handling those additions other than adding additional boolean flags to the various methods like this https://github.com/libsv/go-bt/pull/130/files#diff-8ef97170810c3ede0de2d34df9b1656ec156498c289d7ab2dea5ed8c28e92acdR51 as they will just grow and grow. Perhaps an option and a handler func like map[string]TXExtensionFunc that will get executed depending on tx settings?

@theflyingcodr
Copy link
Contributor

Also thanks for picking up those linter failures, do you think this will be WiP for a bit, I could fix these linter failures in another PR in the mean time to unblock another 2 PRs that are failing because of them. (we run the linter github action which always uses latest linter so it will no doubt pick up stuff the old runs didn't)

bscript/script.go Outdated Show resolved Hide resolved
bscript/script_test.go Outdated Show resolved Hide resolved
input.go Show resolved Hide resolved
tx.go Show resolved Hide resolved
tx.go Show resolved Hide resolved
tx.go Show resolved Hide resolved
tx.go Show resolved Hide resolved
txjson_node_test.go Show resolved Hide resolved
@@ -233,13 +233,32 @@ func (s *Script) ToASM() (string, error) {
parts, err := DecodeParts(*s)
// if err != nil, we will append [error] to the ASM script below (as done in the node).

data := false
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data := false
data := len(*s) > 1 && ((*s)[0] == OpRETURN || ((*s)[0] == OpFALSE && (*s)[1] == OpRETURN))

@mergify mergify bot merged commit 4208458 into libsv:master Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants