Many cleanups to Transaction, see detailed. #76
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Default-ize the sequence rather than use a number, and default to bytes for input. I doubt anybody ever uses this anyways. Remove weird convenience code, and remove wallet logic. Checking a TX's affects on a wallet should be managed by the wallet object. Remove parsing for the weirder SIGHASH types. People use this library for creating SIGHASH_ALL transactions, and I don't see the need to support these other types at the moment since this library's more used for wallets than for hardcore bitcoin tx analysis/creation. They weren't tested anyways. Add note about potentially improving performance by providing pubkey/address. Deriving from the private key is slower, that information should probably be cached by the end user.
Contributor
Author
|
Also worth noting is that I haven't tested my changes to |
Contributor
|
🙋 pushback. Can we add tests to the sighash types that are already supported instead of removing them for the sake of test coverage? |
Contributor
|
I dig the rest of the cleanup. I remember somewhere in the code comment someone mentioned that transaction and wallet has circular dependencies. This resolves that too. |
Contributor
Author
|
I added back the sighash stuff. Let me know if this looks okay and I'll merge. |
weilu
added a commit
that referenced
this pull request
Mar 21, 2014
Many cleanups to Transaction, see detailed.
louisinger
added a commit
to louisinger/liquidjs-lib
that referenced
this pull request
Oct 31, 2022
* wip * fix reissuance test * remove .only * fix after review * remove '!' * reissuanceBlindingArgs * sanity checks * sanity checks * build files * remove check on inflationKeys * better checks * format * clean checks * Update ts_src/psetv2/input.ts Co-authored-by: Pietralberto Mazza <18440657+altafan@users.noreply.github.com> * checks * remove issuance values sanity checks Co-authored-by: altafan <18440657+altafan@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The most controversial thing I did here was remove parsing for weird SIGHASH types. This library is used for creating SIGHASH_ALL txes, and I don't think it needs to support these other types, but I'm OK to get some pushback on this if there's disagreement.
This brings our transaction.js testing coverage up quite a bit:
