Skip to content

Conversation

@gutama
Copy link

@gutama gutama commented Jan 3, 2026

No description provided.

Copilot AI review requested due to automatic review settings January 3, 2026 11:04
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a comprehensive TokenX integration for FabricX and fixes a critical transaction mismatch issue in the endorsement process. The main purpose is to provide a working token management system while resolving bugs that prevented proper endorsement validation.

Key Changes:

  • Fixed FabricX transaction endorsement mismatch by using received RWSet bytes directly instead of re-serializing
  • Fixed sidecar port configuration to use dynamic ports instead of hardcoded values
  • Added comprehensive TokenX integration with support for issuing, transferring, redeeming, and swapping tokens

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
platform/fabricx/core/transaction/transaction.go Core fix: uses received RWSet bytes for endorsement to avoid namespace version mismatches
platform/fabricx/core/vault/interceptor.go Enhanced logging for debugging RWSet serialization and namespace versions
platform/fabric/services/endorser/endorsement.go Added detailed error logging for endorsement result mismatches
integration/nwo/fabricx/extensions/scv2/notificationservice.go Modified to accept dynamic port parameter
integration/nwo/fabricx/extensions/scv2/ext.go Passes dynamic sidecar port to notification service configuration
integration/fabricx/tokenx/views/*.go New token operation views (issue, transfer, redeem, swap, balance, auditor, approver)
integration/fabricx/tokenx/states/states.go Data models for tokens, transaction records, and swap proposals
integration/fabricx/tokenx/topology.go Network topology definition with issuer, auditor, and owner nodes
integration/fabricx/tokenx/api/*.go REST API handlers and OpenAPI specification
integration/fabricx/tokenx/*.md Comprehensive documentation including walkthrough, specification, and task tracking
integration/fabricx/tokenx/*_test.go Integration test suite using Ginkgo
Makefile Added tokenx integration test target and updated linter installation path

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

func (h *redeemHandler) HandleRequest(ctx *server.ReqContext) (interface{}, int) {
req := ctx.Query.(*RedeemRequest)

amount := states.TokenFromFloat(100) // TODO: parse req.Amount
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The hardcoded TODO comment indicates incomplete implementation. The amount parsing from the request is not implemented, which means all redeem requests will attempt to redeem the same hardcoded amount (100) regardless of the requested amount in the API call.

Suggested change
amount := states.TokenFromFloat(100) // TODO: parse req.Amount
amount := states.TokenFromFloat(req.Amount)

Copilot uses AI. Check for mistakes.
Comment on lines 113 to 119
logger.Infof("Querying owner history: type=%s, txType=%s", h.TokenType, h.TxType)

// Get current owner identity
fns, err := fabric.GetDefaultFNS(ctx)
assert.NoError(err, "failed getting FNS")
_ = fns.LocalMembership().DefaultIdentity()

Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The owner variable is assigned but never used. This suggests the history query implementation is incomplete and currently returns empty results.

Suggested change
logger.Infof("Querying owner history: type=%s, txType=%s", h.TokenType, h.TxType)
// Get current owner identity
fns, err := fabric.GetDefaultFNS(ctx)
assert.NoError(err, "failed getting FNS")
_ = fns.LocalMembership().DefaultIdentity()
// Get current owner identity
fns, err := fabric.GetDefaultFNS(ctx)
assert.NoError(err, "failed getting FNS")
owner := fns.LocalMembership().DefaultIdentity()
logger.Infof("Querying owner history for owner [%s]: type=%s, txType=%s", owner, h.TokenType, h.TxType)

Copilot uses AI. Check for mistakes.
Makefile Outdated
install-linter-tool: ## Install linter in $(go env GOPATH)/bin
@echo "Installing golangci Linter"
@curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.4.0
@curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b ~/.local/bin v2.4.0
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The golangci-lint installer path should use a shell variable expansion $(shell go env GOPATH)/bin instead of the literal string ~/.local/bin to ensure consistency across different development environments and to match the pattern used in line 41.

Suggested change
@curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b ~/.local/bin v2.4.0
@curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(shell go env GOPATH)/bin v2.4.0

Copilot uses AI. Check for mistakes.
Comment on lines 591 to 594
var serErr error
rawTx, serErr = rwset.Bytes()
if serErr != nil {
return nil, errors.WithMessagef(serErr, "error serializing rws for [%s]", t.ID())
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The variable serErr is unnecessarily introduced. The error can be returned directly using err as in the surrounding code, which is more consistent with the codebase style.

Suggested change
var serErr error
rawTx, serErr = rwset.Bytes()
if serErr != nil {
return nil, errors.WithMessagef(serErr, "error serializing rws for [%s]", t.ID())
rawTx, err = rwset.Bytes()
if err != nil {
return nil, errors.WithMessagef(err, "error serializing rws for [%s]", t.ID())

Copilot uses AI. Check for mistakes.
Amount: i.Amount,
Owner: i.Recipient,
IssuerID: "issuer", // Simple identifier
CreatedAt: time.Time{},
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The CreatedAt field is initialized with a zero time value time.Time{} instead of the current time using time.Now(). This is inconsistent with other token creation in the codebase (see transfer.go line 86, 99) and will result in tokens having an invalid timestamp.

Suggested change
CreatedAt: time.Time{},
CreatedAt: time.Now(),

Copilot uses AI. Check for mistakes.
Comment on lines 67 to 70
// For now, we demonstrate the pattern
_ = qs
_ = vault
_ = owner
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

Unused variables qs, vault, and owner are assigned but never used. This suggests the balance query implementation is incomplete and currently returns empty results.

Suggested change
// For now, we demonstrate the pattern
_ = qs
_ = vault
_ = owner
// For now, we demonstrate the pattern. Currently, we only log the context.
logger.Infof(
"[BalanceView] Query context - owner: %+v, queryService: %T, vault: %T",
owner, qs, vault,
)

Copilot uses AI. Check for mistakes.
TokenType: i.TokenType,
Amount: i.Amount,
To: i.Recipient,
Timestamp: time.Time{},
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The Timestamp field is initialized with a zero time value time.Time{} instead of the current time using time.Now(). This is inconsistent with other transaction records in the codebase (see transfer.go line 112) and will result in records having invalid timestamps.

Copilot uses AI. Check for mistakes.
req := ctx.Query.(*IssueRequest)

// Convert amount string to uint64 (would parse decimal in production)
amount := states.TokenFromFloat(1000) // TODO: parse req.Amount
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The hardcoded TODO comment indicates incomplete implementation. The amount parsing from the request is not implemented, which means all issue requests will create tokens with the same hardcoded amount (1000) regardless of the requested amount in the API call.

Copilot uses AI. Check for mistakes.
func (h *transferHandler) HandleRequest(ctx *server.ReqContext) (interface{}, int) {
req := ctx.Query.(*TransferRequest)

amount := states.TokenFromFloat(100) // TODO: parse req.Amount
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

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

The hardcoded TODO comment indicates incomplete implementation. The amount parsing from the request is not implemented, which means all transfer requests will attempt to transfer the same hardcoded amount (100) regardless of the requested amount in the API call.

Copilot uses AI. Check for mistakes.
@mbrandenburger
Copy link
Member

@gutama Thank you for creating this PR. Can you please have a look at the DCO check and fix it.

Signed-off-by: gutama <ginanjar.utama@gmail.com>
@gutama gutama force-pushed the fix/tokenx-integration branch from 25e23e2 to d534a52 Compare January 6, 2026 22:18
@gutama
Copy link
Author

gutama commented Jan 6, 2026

DCO check passed, thank you

@adecaro
Copy link
Contributor

adecaro commented Jan 7, 2026

I'm wondering if the TokenX app should be better positioned in the fabric-samples rather than here.
@gutama @mbrandenburger , what do you think?

@adecaro
Copy link
Contributor

adecaro commented Jan 7, 2026

@gutama , is this PR fully generated by AI?

@gutama
Copy link
Author

gutama commented Jan 7, 2026

Actually at first I only want to learn how to use fabricx by extending simple and iou sample, so TokenX uses FabricX and FSC state management directly. Somehow I can't run integration test successfully that's why I put PR, because the code need changes in other places.

Fully generated by AI and a lot of debugging.

Signed-off-by: Ginanjar Utama <ginanjar.utama@gmail.com>
SPDX-License-Identifier: Apache-2.0
*/

package api
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if we really should add this api package as part of an integration test here. For a real FSC-based application, it makes sense to me, however, as part of the integration test suite I believe we aim to keep the tests small.

Issue
}

func (i *IssueView) Call(ctx view.Context) (interface{}, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I can see that this view is called via the integration tests. @gutama Do you plan to also add tests for the other views, including, transfer, redeem, etc ... ?

func (e *Extension) GenerateArtifacts() {
generateQSExtension(e.network)
generateNSExtension(e.network)
generateNSExtension(e.network, e.sidecar.Ports[fabric_network.ListenPort])
Copy link
Member

Choose a reason for hiding this comment

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

This is a good improvement!

@@ -0,0 +1,1008 @@
# TokenX - Token Management System Specification
Copy link
Member

Choose a reason for hiding this comment

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

I think this a great spec! However, I have the feeling it's a bit beyond the scope of an integration test. I could think about create a dedicated lab to continue development of this application, or work towards a "fabric-x" sample. What do you think?

Comment on lines +23 to +35
### 1. Endorsement Mismatch (Fixed)

**Issue**: The FabricX RWSet serialization includes local namespace versions. When the approver node re-serialized the RWSet, it used its own local versions which differed from the issuer's, causing a hash mismatch.

**Fix**: Modified `platform/fabricx/core/transaction/transaction.go` to check if `t.RWSet` is populated (received from issuer) and use it directly instead of re-serializing.

### 2. Sidecar Port Mismatch (Fixed)

**Issue**: The integration test hung at "Post execution for FSC nodes...". Debugging revealed the Sidecar container was launching on a dynamic port (e.g., 5420), but the client configuration was hardcoded to connect to `5411`.

**Fix**:
- Updated `integration/nwo/fabricx/extensions/scv2/notificationservice.go`: `generateNSExtension` now accepts a port argument.
- Updated `integration/nwo/fabricx/extensions/scv2/ext.go`: `GenerateArtifacts` retrieves the correct `fabric_network.ListenPort` from the sidecar configuration and passes it to `generateNSExtension`.
Copy link
Member

Choose a reason for hiding this comment

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

Great job identifying these issue. Let's work on getting these fixes in.

@mbrandenburger
Copy link
Member

@gutama Thanks again for your PR. I left a few comments (see above). I really like your proposal for another integration test that demonstrates a full token system built with FPC and Fabric-x, however, I am also hesitating if the FSC integration tests are the best home for this application. Ideally, we can refine the scope of this PR and remove functionality which is not yet tests (e.g., transfer, redeem, etc) or not needed for the integration test (e.g., the SPEC, api package, ...). WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants