Skip to content

Aggregated Protobuf Best Practices Analysis & Improvement Plan #1622 - #1717

Merged
adecaro merged 5 commits into
mainfrom
fabric-token-sdk-1622
Jun 26, 2026
Merged

Aggregated Protobuf Best Practices Analysis & Improvement Plan #1622#1717
adecaro merged 5 commits into
mainfrom
fabric-token-sdk-1622

Conversation

@adecaro

@adecaro adecaro commented May 18, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@adecaro
adecaro marked this pull request as draft May 18, 2026 11:10
@adecaro
adecaro force-pushed the fabric-token-sdk-1622 branch 3 times, most recently from 374aaaa to ddc36bd Compare May 20, 2026 12:24
@adecaro adecaro self-assigned this May 22, 2026
@adecaro
adecaro requested a review from alexandrosfilios May 22, 2026 05:31
@adecaro adecaro added this to the Q2/26 milestone May 22, 2026
@adecaro adecaro linked an issue May 22, 2026 that may be closed by this pull request
Aggregated Protobuf Best Practices Analysis & Improvement Plan #1622
Closed
@adecaro
adecaro force-pushed the fabric-token-sdk-1622 branch 3 times, most recently from 0bd061c to 46eb8b9 Compare May 23, 2026 05:03
@adecaro
adecaro force-pushed the fabric-token-sdk-1622 branch 11 times, most recently from f46c7d9 to 142e48a Compare May 30, 2026 05:17
@adecaro adecaro mentioned this pull request Jun 1, 2026
Closed
4 tasks
Storm1289 pushed a commit to Storm1289/fabric-token-sdk-lfx that referenced this pull request Jun 1, 2026
Storm1289
revert(audit): drop auditor changes; auditor rewrite moves to LFDT-Pa…
519b689 
…nurus#1717

The protobuf-level rewrite in LFDT-Panurus#1717 supersedes the auditor changes
originally bundled in this PR. Keep only the driver-level Match,
MatchInputs, and ValidateReceivers refactor here; auditor input
correctness will be re-introduced as part of LFDT-Panurus#1717.

Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
@Storm1289

Storm1289 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Hi @adecaro, went through the new auditor.

Input commitments aren't re-checked -validateTransferInputs (auditor.go:403) only verifies owner/sender bytes; actionInput.Token.Data is never recomputed and compared the way InspectOutput does for outputs (auditor.go:585). A tampered input commitment would pass audit, which is what #998 was about.

A few smaller things:

  • Single sender hardcoded at auditor.go:418 (len(Senders) != 1) - breaks multi-sig inputs?
  • TokenID check is conditional at auditor.go:439 (if TokenID != nil) - nil TokenID silently skips. Hard error instead?
  • Empty receiver identity silently accepted in the transfer path at auditor.go:561 - InspectIdentity then fails late with "identity is nil". Fail early?

Thanks 🙏

@adecaro

adecaro commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Hi @adecaro, went through the new auditor.

Input commitments aren't re-checked -validateTransferInputs (auditor.go:403) only verifies owner/sender bytes; actionInput.Token.Data is never recomputed and compared the way InspectOutput does for outputs (auditor.go:585). A tampered input commitment would pass audit, which is what #998 was about.

A few smaller things:

  • Single sender hardcoded at auditor.go:418 (len(Senders) != 1) - breaks multi-sig inputs?
  • TokenID check is conditional at auditor.go:439 (if TokenID != nil) - nil TokenID silently skips. Hard error instead?
  • Empty receiver identity silently accepted in the transfer path at auditor.go:561 - InspectIdentity then fails late with "identity is nil". Fail early?

Thanks 🙏

Great catches, @Storm1289. Indeed. Those points need to be addressed 🙏

@adecaro
adecaro force-pushed the fabric-token-sdk-1622 branch from 862be46 to ad6b562 Compare June 2, 2026 04:34
@adecaro adecaro removed this from the Q2/26 milestone Jun 2, 2026
@adecaro
adecaro requested a review from AkramBitar June 3, 2026 05:56
@adecaro
adecaro force-pushed the fabric-token-sdk-1622 branch 3 times, most recently from 01436c3 to 789125d Compare June 5, 2026 04:57
AkramBitar
AkramBitar requested changes Jun 19, 2026
View reviewed changes
Comment thread docs/driverapi.md
Comment thread docs/driverapi.md
Comment thread token/core/common/auditor.go
Comment thread token/core/common/auditor.go Outdated
Comment thread token/core/common/auditor.go Outdated
Comment thread token/core/common/auditor.go Outdated
Comment thread token/core/common/auditor.go
Comment thread token/core/common/validator.go
Comment thread token/core/common/validator.go
Comment thread token/core/zkatdlog/nogh/v1/testutils/support.go Outdated
@AkramBitar AkramBitar linked an issue Jun 23, 2026 that may be closed by this pull request
dlog v1 finalization: an extra signer in the metadata should be an AuditableIdentity #993
Closed
@adecaro
adecaro force-pushed the fabric-token-sdk-1622 branch from 58dd6b1 to 6d268de Compare June 24, 2026 07:24
@adecaro
adecaro marked this pull request as ready for review June 24, 2026 07:24
@adecaro
adecaro force-pushed the fabric-token-sdk-1622 branch 3 times, most recently from 499316b to ef56c56 Compare June 25, 2026 09:09
AkramBitar
AkramBitar approved these changes Jun 25, 2026
View reviewed changes

@AkramBitar AkramBitar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM
Great work @adecaro thanks a million.

@adecaro
adecaro force-pushed the fabric-token-sdk-1622 branch 2 times, most recently from 4557090 to 151fafc Compare June 26, 2026 04:32
adecaro added 4 commits June 26, 2026 09:33
@adecaro
protobuf refactoring + cascading services refactoring and update
fafffa0 
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
@adecaro
cleanup fix
b4a20f4 
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
@adecaro
commnets
b19beed 
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
@adecaro
fix unit-tests
7e7d71d 
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
@adecaro
adecaro force-pushed the fabric-token-sdk-1622 branch from fca4b4d to 7e7d71d Compare June 26, 2026 06:33
@adecaro
unit-test fix
b1f4d3c 
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
@adecaro
adecaro merged commit a3914d4 into main Jun 26, 2026
56 checks passed
@adecaro
adecaro deleted the fabric-token-sdk-1622 branch June 26, 2026 08:58
This was referenced Jul 13, 2026
Closed
Merged
adecaro pushed a commit to YanYuanFE/panurus that referenced this pull request Jul 14, 2026
@YanYuanFE @adecaro
fix(token): preserve auditor signatures in Request.SetSignatures
e040806 
Since LFDT-Panurus#1717 action and auditor signatures share the unified
Actions.Signatures slice, but SetSignatures still assigned the whole
field, silently dropping auditor signatures attached beforehand via
AddAuditorSignature. Endorsement then fails with 'auditor signatures
missing' when auditing runs before endorsement collection.

Preserve existing auditor entries when assigning action signatures,
restoring the pre-LFDT-Panurus#1717 order-independence of the two calls.

Fixes LFDT-Panurus#1897

Signed-off-by: YanYuan <yan950414@gmail.com>
YanYuanFE added a commit to YanYuanFE/panurus that referenced this pull request Jul 14, 2026
@YanYuanFE
fix(token): preserve auditor signatures in Request.AppendSignatures
69e5bb4 
Since LFDT-Panurus#1717 action and auditor signatures share the unified
Actions.Signatures slice, but SetSignatures still assigned the whole
field, silently dropping auditor signatures attached beforehand via
AddAuditorSignature. Endorsement then fails with 'auditor signatures
missing' when auditing runs before endorsement collection.

Preserve existing auditor entries when assigning action signatures,
restoring the pre-LFDT-Panurus#1717 order-independence of the two calls, and rename
SetSignatures to AppendSignatures so the name states that contract.

Fixes LFDT-Panurus#1897

Signed-off-by: YanYuan <yan950414@gmail.com>
YanYuanFE added a commit to YanYuanFE/panurus that referenced this pull request Jul 14, 2026
@YanYuanFE
fix(token): preserve auditor signatures in Request.AppendSignatures
75fb814 
Since LFDT-Panurus#1717 action and auditor signatures share the unified
Actions.Signatures slice, but SetSignatures still assigned the whole
field, silently dropping auditor signatures attached beforehand via
AddAuditorSignature. Endorsement then fails with 'auditor signatures
missing' when auditing runs before endorsement collection.

Append the action signatures to the ones already attached, restoring
the pre-LFDT-Panurus#1717 order-independence of the two calls, and rename
SetSignatures to AppendSignatures so the name states that contract.

Fixes LFDT-Panurus#1897

Signed-off-by: YanYuan <yan950414@gmail.com>
adecaro pushed a commit to YanYuanFE/panurus that referenced this pull request Jul 14, 2026
@YanYuanFE @adecaro
fix(token): preserve auditor signatures in Request.AppendSignatures
bae6de7 
Since LFDT-Panurus#1717 action and auditor signatures share the unified
Actions.Signatures slice, but SetSignatures still assigned the whole
field, silently dropping auditor signatures attached beforehand via
AddAuditorSignature. Endorsement then fails with 'auditor signatures
missing' when auditing runs before endorsement collection.

Append the action signatures to the ones already attached, restoring
the pre-LFDT-Panurus#1717 order-independence of the two calls, and rename
SetSignatures to AppendSignatures so the name states that contract.

Fixes LFDT-Panurus#1897

Signed-off-by: YanYuan <yan950414@gmail.com>
adecaro pushed a commit to YanYuanFE/panurus that referenced this pull request Jul 14, 2026
@YanYuanFE @adecaro
fix(token): preserve auditor signatures in Request.AppendSignatures
e6ce76e 
Since LFDT-Panurus#1717 action and auditor signatures share the unified
Actions.Signatures slice, but SetSignatures still assigned the whole
field, silently dropping auditor signatures attached beforehand via
AddAuditorSignature. Endorsement then fails with 'auditor signatures
missing' when auditing runs before endorsement collection.

Append the action signatures to the ones already attached, restoring
the pre-LFDT-Panurus#1717 order-independence of the two calls, and rename
SetSignatures to AppendSignatures so the name states that contract.

Fixes LFDT-Panurus#1897

Signed-off-by: YanYuan <yan950414@gmail.com>
adecaro pushed a commit to YanYuanFE/panurus that referenced this pull request Jul 15, 2026
@YanYuanFE @adecaro
fix(token): preserve auditor signatures in Request.AppendSignatures
88b6d35 
Since LFDT-Panurus#1717 action and auditor signatures share the unified
Actions.Signatures slice, but SetSignatures still assigned the whole
field, silently dropping auditor signatures attached beforehand via
AddAuditorSignature. Endorsement then fails with 'auditor signatures
missing' when auditing runs before endorsement collection.

Append the action signatures to the ones already attached, restoring
the pre-LFDT-Panurus#1717 order-independence of the two calls, and rename
SetSignatures to AppendSignatures so the name states that contract.

Fixes LFDT-Panurus#1897

Signed-off-by: YanYuan <yan950414@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkramBitar AkramBitar AkramBitar approved these changes
@alexandrosfilios alexandrosfilios Awaiting requested review from alexandrosfilios

Assignees

@adecaro adecaro

Labels

protobuf protocol

Projects

None yet

Milestone

Q3/26

Development

Successfully merging this pull request may close these issues.

Aggregated Protobuf Best Practices Analysis & Improvement Plan dlog v1 finalization: an extra signer in the metadata should be an AuditableIdentity

3 participants

@adecaro @Storm1289 @AkramBitar