Conversation
🟡 Heimdall Review Status
|
|
This looks good to me, but I'll defer to @jingweicb or @GeekArthur for the changes to |
|
Thank you, @potterbm-cb I'll keep an eye out for any updates here 👍 |
|
Hi @sleepdefic1t , how did you test the pr? can you add the testing in pr discriptions? |
| // the max fee will set an upper bound on the suggested fee (regardless of the multiplier provided). | ||
| // it MUST be fetched in the call to `/construction/metadata`. | ||
| type ConstructionPreprocessRequest struct { | ||
| NetworkIdentifier *NetworkIdentifier `json:"network_identifier"` |
There was a problem hiding this comment.
why removed SuggestedFeeMultiplier and MaxFee?
There was a problem hiding this comment.
The reason codegen.sh removes those two fields is because your team's fix was never merged.
There was a problem hiding this comment.
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.
can you add them back manually ?
I'm not sure I follow, @jingweicb This PR only updates the spec version used by the SDK, what should be tested? Our proposal to add BIP-340 support was shared by @GeekArthur here: Perhaps that discussion can provide clarity. |
|
Just following up. I'm ready to proceed whenever you are. |
| EcdsaRecovery SignatureType = "ecdsa_recovery" | ||
| Ed25519 SignatureType = "ed25519" | ||
| Schnorr1 SignatureType = "schnorr_1" | ||
| SchnorrBip340 SignatureType = "schnorr_bip340" |
There was a problem hiding this comment.
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.
CAN I have more context of why we add it ?
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
if you wanna support a new type, you can test in at derive endpoint
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.
| // the max fee will set an upper bound on the suggested fee (regardless of the multiplier provided). | ||
| // it MUST be fetched in the call to `/construction/metadata`. | ||
| type ConstructionPreprocessRequest struct { | ||
| NetworkIdentifier *NetworkIdentifier `json:"network_identifier"` |
There was a problem hiding this comment.
can you add them back manually ?
Manually revert ConstructionPreprocessRequest types file per @jingweicb
Hi @GeekArthur , I think this pr matches the discussion raised by you, can you take a look? I am ok once the pr can add the missing fields back |
|
Hello @jingweicb @GeekArthur I'd like to keep the momentum going here so that we may continue with adding BIP-340 support. Please let me know how you'd like to proceed. |
|
Hello @jingweicb @GeekArthur Just following up again. Please let me know how you'd like to proceed with this PR. |
|
Hello again, @GeekArthur and @jingweicb, Please let me know how to proceed with this. It’s been over two years since the work to add BIP-340 support was greenlit, and I’d greatly appreciate further direction on moving this forward. |
|
Hello @GeekArthur and @jingweicb, Any update on this PR? We are still waiting :( |
5 participants
Fixes # .
Motivation
Schnorr BIP-340 support in
mesh-sdk-gofirst requires the addition of the appropriate curve and signature types found in the current specifications release.Solution
Update mesh specification to 1.4.15 as discussed in #500
CurveTypeandSignatureType