Skip to content
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.

Sign up for GitHub

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

Fix transferFrom function in erc-1155 sample so that we only delete token balances after… #1060

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix transferFrom function in erc-1155 sample so that we only delete token balances after… #1060

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 42 additions & 22 deletions token-erc-1155/chaincode-go/chaincode/contract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ package chaincode
import (
"encoding/json"
"fmt"
"github.com/hyperledger/fabric-protos-go/ledger/queryresult"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -949,6 +950,8 @@ func removeBalance(ctx contractapi.TransactionContextInterface, sender string, i
}
defer balanceIterator.Close()

var deferredDeletions = []func() error{}

// Iterate over keys that store balances and add them to partialBalance until
// either the necessary amount is reached or the keys ended
for balanceIterator.HasNext() && partialBalance < neededAmount {
Expand All @@ -972,47 +975,64 @@ func removeBalance(ctx contractapi.TransactionContextInterface, sender string, i
selfRecipientKeyNeedsToBeRemoved = true
selfRecipientKey = queryResponse.Key
} else {
err = ctx.GetStub().DelState(queryResponse.Key)
if err != nil {
return fmt.Errorf("failed to delete the state of %v: %v", queryResponse.Key, err)
}
deferredDeletions = append(deferredDeletions, deferredDelete(ctx, queryResponse))
Copy link
Contributor

Choose a reason for hiding this comment

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

DelState() won't delete at this time. It will build a write set that will perform the deletes upon successful completion of the chaincode and validation of the transaction. It is therefore implicitly deferred already. Introducing an explicit defer in my opinion makes the chaincode more confusing to the user, and gives the wrong idea that there is some type of explicit deferral possible in the chaincode programming model.

I see your point however about making the implicit deferral more obvious to the user. I'd suggest that the better way to educate the user would be to add a comment at the existing DelState() call indicating that the DelState() calls will get collected in the write set and will only be executed if the remaining checks are passed and the transaction gets validated. This will educate the user on the chaincode programming model and keep the code more readable.

Copy link
Author

@robevansuk robevansuk Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This makes sense but when testing with shimtest.MockStub, the change is made at the time its executed and is not part of any writeSet that will ignore the deletion... this makes testing this function impossible using a TDD approach, unless I change my assumption to "delete these token funds even when there aren't enough available".

A better solution might be to push the deletion further down in the chain of events but I'd like confirmation whether this would be worthwhile/acceptable before I make the change.

}
}

if partialBalance < neededAmount {
return fmt.Errorf("sender has insufficient funds for token %v, needed funds: %v, available fund: %v", tokenId, neededAmount, partialBalance)
} else if partialBalance > neededAmount {
// Send the remainder back to the sender
remainder, err := sub(partialBalance, neededAmount)
if err != nil {
return err
}

if selfRecipientKeyNeedsToBeRemoved {
// Set balance for the key that has the same address for sender and recipient
err = setBalance(ctx, sender, sender, tokenId, remainder)
} else {
// enough token funds have been found to perform the update
// now we can delete the token entries to supply updated token balances
for _, deleteFn := range deferredDeletions {
err := deleteFn()
if err != nil {
return err
}
} else {
err = addBalance(ctx, sender, sender, tokenId, remainder)
}
if partialBalance > neededAmount {
// Send the remainder back to the sender
remainder, err := sub(partialBalance, neededAmount)
if err != nil {
return err
}
}

} else {
// Delete self recipient key
err = ctx.GetStub().DelState(selfRecipientKey)
if err != nil {
return fmt.Errorf("failed to delete the state of %v: %v", selfRecipientKey, err)
if selfRecipientKeyNeedsToBeRemoved {
// Set balance for the key that has the same address for sender and recipient
err = setBalance(ctx, sender, sender, tokenId, remainder)
if err != nil {
return err
}
} else {
err = addBalance(ctx, sender, sender, tokenId, remainder)
if err != nil {
return err
}
}

} else {
// Delete self recipient key
err = ctx.GetStub().DelState(selfRecipientKey)
if err != nil {
return fmt.Errorf("failed to delete the state of %v: %v", selfRecipientKey, err)
}
}
}
}

return nil
}

func deferredDelete(ctx contractapi.TransactionContextInterface, queryResponse *queryresult.KV) func() error {
return func() error {
err := ctx.GetStub().DelState(queryResponse.Key)
if err != nil {
return fmt.Errorf("failed to delete the state of %v: %v", queryResponse.Key, err)
}
return nil
}
}

func emitTransferSingle(ctx contractapi.TransactionContextInterface, transferSingleEvent TransferSingle) error {
transferSingleEventJSON, err := json.Marshal(transferSingleEvent)
if err != nil {
Expand Down