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

Remove unneeded argument from toDER method #3185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

msalcala11
Copy link
Contributor

No description provided.

Copy link
Member

@cmgustavo cmgustavo left a comment

Choose a reason for hiding this comment

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

ACK!

Copy link
Collaborator

@kajoseph kajoseph left a comment

Choose a reason for hiding this comment

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

I like the direction you're going here (using the class var as the source of truth), but there are some dangling parameters around the code base. For example, in Signature.prototype.toTxFormat and in MultiSigInput.prototype._createSignatures (and the calling stack of the latter).

For reference, here is the commit where a lot of this was implemented: e00577a

If you want to really go ham, I think the signingMethod param can be scrubbed out of a lot of the codebase since the sighash.js verify function can have the signingMethod param removed (and all implementations) since the signing method is implied by sig.isSchnorr), but that's a little beyond the scope of this MR so I'll leave that up to you whether or not you want to follow that rabbit hole.

Again, I like where you're going with this change. I'd like to see this pattern used more throughout the rest of the code.

@msalcala11
Copy link
Contributor Author

Thanks for sharing the original commit and additional context. Would be great to eventually do a more thorough refactor and remove more unneeded params from the codebase. This PR is merely meant to be a quick fix for a bug I encountered (this.isSchnorr is currently being overwritten to undefined if someone calls .set without isSchnorr included as part of the params obj)

@kajoseph
Copy link
Collaborator

kajoseph commented Jun 9, 2021

Oh ok, I was thrown off by the title of the PR being about removing the signingMethod param in the toBuffer/toDER function. That's fine if you're just wanting to change the one line in set. Removing the toDER param would need a little more attention if you want to continue with that.

@msalcala11
Copy link
Contributor Author

Well it looks like the author of the original commit encountered this same bug and added the siginingMethod param to the toDER method as quick patch, but addressing the root cause (ensuring that this.isSchnorr does not get wiped out by a .set call without obj.isSchnorr set) makes toDER's signingMethod param unnecessary.

@kajoseph
Copy link
Collaborator

kajoseph commented Jun 9, 2021

Yep, that's all good. We'll just need to finish the job instead of leaving dangling params such as in Signature.prototype.toTxFormat, etc.

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