HashEscrow: separate hash-based escrow identity flow (#1657)#1678
HashEscrow: separate hash-based escrow identity flow (#1657)#1678Flamki wants to merge 19 commits into
Conversation
|
Hi @Flamki , thanks for submitting this PR and for your effort 🙏 I think it is confusing adding that flag to the HTLC-based identity because it introduces a new path that is outside the pure scope of the HTLC. I think @KElkhiyaoui had in mind something much simpler that doesn't even name Alice or Bob but just the hash. Who has the pre-image can spend the token. @KElkhiyaoui , please, can you clarify this? |
Thanks for the review. I'm currently re-evaluating the approach and exploring a direction that better aligns with the intended HTLC semantics. I'll refine the design based on the feedback and update the PR accordingly. |
9a7fae1 to
0431933
Compare
|
Implemented and pushed an update aligned with the latest direction. What changed in this push:
Local validation:
Both passed locally. |
8c64f69 to
a359faf
Compare
…Panurus#1657) Signed-off-by: Flamki <9833ayush@gmail.com>
Signed-off-by: Flamki <9833ayush@gmail.com>
a359faf to
56749b4
Compare
|
Hi @Flamki , thanks for this big effort. I'll ask @KElkhiyaoui to also review. I would already ask to enhance the integration test to exercise the new identity as done for HTLC. Thanks much 🙏 |
Signed-off-by: Flamki <9833ayush@gmail.com>
|
@adecaro @KElkhiyaoui The new flow covers:
I also added the small integration view/helpers needed to exercise this path in the same style as the existing HTLC tests. Local checks passed:
|
|
Hi @KElkhiyaoui , I need a review from you here to make sure the PR matches what you had in mind. Thanks much 🙏 @Flamki , thanks for your effort 🙏 |
| senderWallet := hashescrow.GetWallet(context, hv.Wallet, token.WithTMSID(hv.TMSID)) | ||
| assert.NotNil(senderWallet, "sender wallet [%s] not found", hv.Wallet) | ||
|
|
||
| preImages, err := tx.LockWithPreImages( |
There was a problem hiding this comment.
The idea is that the preimage are not known at time of lock. The sender and the recipient are given hashes from the escrow through side channels. If the escrow is trusted, we can assume for now that the hashes are correctly computed. We can also imagine that the escrow provides proofs that the hashes are well computed.
The next step could be to have a view where the sender and the recipient are provided with the hshaes.
So the lock method should just take the hashes directly and not compute them within.
| ctx.CountMetadataKey(metadataKey) | ||
| } | ||
| } | ||
| if owner.Type == hashescrow.ScriptType { |
There was a problem hiding this comment.
Instead of having this code under TransferHTLCValidate, you can create a new dedicated method that we add to the validator at time of instantiation.
| ctx.CountMetadataKey(metadataKey) | ||
| } | ||
| } | ||
| if owner.Type == hashescrow.ScriptType { |
There was a problem hiding this comment.
It would be better to create a dedicated method to validate HashEscrow scripts.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if sender.Type != ScriptType { |
There was a problem hiding this comment.
Maybe instead of calling ScriptType, we call it HashEscrow, so that it's clear what it refers to?
| if len(claim.Preimage) == 0 { | ||
| return "", nil, errors.New("expected a valid claim preImage") | ||
| } | ||
| resolvedOwner, image, err := script.ResolveRecipientForPreImage(claim.Preimage) |
| if options.Attributes != nil { | ||
| // backward compatibility | ||
| boxed, ok := options.Attributes["hashescrow.hash"] | ||
| if ok { |
There was a problem hiding this comment.
Why do we need this entry?
| preImages := &PreImages{} | ||
| var err error | ||
|
|
||
| recipientHashInfo, recipientPreImage, err := buildHashInfo(recipientHash, hashFunc, hashEncoding) |
There was a problem hiding this comment.
As mentioned before, the computation of the hashes will precede the call of lock view.
| } | ||
|
|
||
| // MetadataClaimKeyCheck validates claim metadata and returns the matched metadata key. | ||
| func MetadataClaimKeyCheck(action Action, script *hashescrow.Script, sig []byte) (string, []byte, error) { |
There was a problem hiding this comment.
Maybe this method should be called ClaimMetadataCheck. The current name suggests that we are checking the key.
| if len(claim.Preimage) == 0 { | ||
| return "", nil, errors.New("expected a valid claim preImage") | ||
| } | ||
| resolvedOwner, image, err := script.ResolveRecipientForPreImage(claim.Preimage) |
There was a problem hiding this comment.
It would be better to separate duties. That is the method checking the metadata takes as input the pre-image and the hash directly and checks that the value matches. Resolving recipients could be directly used in validator transfer.
|
|
||
| // check metadata | ||
| sigma := ctx.Signatures[i] | ||
| metadataKey, resolvedOwner, err := hashescrow2.MetadataClaimKeyCheck(ctx.TransferAction, script, sigma) |
There was a problem hiding this comment.
For better readability, I would recommend that we use a method dedicated to resolving the identity o the recipient and returning the pre-image.
|
Hi @Flamki , could you address @KElkhiyaoui 's comments. Many thanks for the effort 🙏 |
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
Signed-off-by: Flamki <111645429+Flamki@users.noreply.github.com>
| continue | ||
| } | ||
| if len(ctx.InputTokens) != 1 || len(ctx.TransferAction.GetOutputs()) != 1 { | ||
| return ErrInvalidHTLCAction |
There was a problem hiding this comment.
| return ErrInvalidHTLCAction | |
| return ErrInvalidHashEscrowAction |
|
|
||
| out, ok := ctx.TransferAction.GetOutputs()[0].(*token.Token) | ||
| if !ok || out == nil { | ||
| return ErrHTLCOutputNotFound |
There was a problem hiding this comment.
| return ErrHTLCOutputNotFound | |
| return ErrHashEscrowOutputNotFound |
| return &AuditDeserializer{AuditInfoDeserializer: auditInfoDeserializer} | ||
| } | ||
|
|
||
| func (a *AuditDeserializer) DeserializeAuditInfo(ctx context.Context, identity driver.Identity, raw []byte) (idriver.AuditInfo, error) { |
There was a problem hiding this comment.
I expect this method to return the auditInfo for the script (i.e., ScriptInfo).
| return nil, errors.New("owner of output token does not correspond to sender or recipient in hash escrow request") | ||
| } | ||
|
|
||
| func ClaimFromSignature(sig []byte) (*hashescrow.ClaimSignature, error) { |
There was a problem hiding this comment.
We can add methods Serialize and Deserialize to ClaimSignature.
|
|
||
| // ResolveRecipientForPreImage is kept for backward compatibility with existing call sites. | ||
| func (s *Script) ResolveRecipientForPreImage(preImage []byte) (view.Identity, []byte, error) { | ||
| owner, hash, _, err := s.ResolveOwnerAndHashForPreimage(preImage) |
There was a problem hiding this comment.
Instead of owner we can use Recipient. Maybe to avoid confusion between recipient who gets the token by the end of claim and recipient in the script, we call it claimant.
|
Hello @Flamki, First, I would like to thank you for your efforts on this PR. I really appreciate your contribution. Second, I was wondering if you have had a chance to address @KElkhiyaoui’s comments? Regards, |
|
Hi @Flamki , if it is okay for you, I'm gonna fork your branch (so keeping the history) and continue the work. Would that be okay for you? Thanks a lot for this initial effort 🙏 |
Summary
This PR reworks #1657 by introducing a separate hashescrow identity flow instead of extending HTLC behavior.
The implementation was adjusted after review feedback to keep semantics clear:
Design
hashescrow.This keeps escrow non-custodial: escrow controls preimage disclosure, but does not own locked tokens.
What Changed
Added new interop package:
token/services/interop/hashescrowAdded new identity package:
token/services/identity/interop/hashescrowAdded identity mappings:
token/driver/wallet.gotoken/services/identity/typed.gotoken/services/identity/marshal/marshal.goWired hashescrow into both token drivers (fabtoken and zkatdlog):
Extended transfer validators (fabtoken and zkatdlog) to validate hashescrow spend/lock semantics and metadata.
Tests
Added unit coverage for:
Validation
I validated this change locally with:
go test -count=1 ./token/services/interop/hashescrow ./token/services/identity/interop/hashescrow ./token/core/fabtoken/v1/driver ./token/core/zkatdlog/nogh/v1/driver ./token/core/fabtoken/v1/validator ./token/core/zkatdlog/nogh/v1/validator ./token/services/identity/marshalgolangci-lint run --timeout=30mNotes
mainand resolved thetoken/driver/wallet.goconflict.Fixes #1657