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.
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
Update spec to 1.4.15 #509
base: master
Are you sure you want to change the base?
Update spec to 1.4.15 #509
Changes from 1 commit
88d8e63
466ffc2
dba6a3f
092fb69
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why removed SuggestedFeeMultiplier and MaxFee?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jingweicb
The reason
codegen.sh
removes those two fields is because your team's fix was never merged.#479
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jingweicb
Noting that #479 (your team’s fix) is unmerged and out of scope here. Can we mark this resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add them back manually ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jingweicb
Reverted: 092fb69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sleepdefic1t , CAN I have more context of why we add it ?
if you wanna support a new type, you can test in at derive endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jingweicb
The addition of these types is necessary because the construction and handling of BIP-340 signatures and public keys differ from other signature types.
To reiterate, all of this was previously discussed and approved here: coinbase/mesh-specifications#113
Perhaps I'm still misunderstanding.
This PR only adds the base signature and curve types as they relate to the specification, it's not intended to be a full or partial implementation of BIP-340.
It was discussed and agreed upon that a subsequent PR would handle the actual implementation of BIP-340, which would then utilise these new types.
Could you clarify where you propose adding a test? Without further implementation, testing doesn’t seem possible based solely on the spec update.