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.

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

Conversation

robevansuk
Copy link

@robevansuk robevansuk commented Jun 21, 2023

… we've verified the funds are available first - not before

Deleting the state before the necessary funds are identified first results in funds disappearing without a recipient receiving them since the transaction then results in an error state being returned.

This update defers the deleteState calls so that they do not occur if partialBalance < neededAmount is true (not enough funds available)

@robevansuk robevansuk requested a review from a team as a code owner June 21, 2023 23:40
@robevansuk robevansuk changed the title Fix transferFrom function so that we only delete token balances after… Fix transferFrom function in erc-1155 sample so that we only delete token balances after… Jun 21, 2023
… we've verified the funds are available first - not before

Signed-off-by: Rob Evans <[email protected]>
@robevansuk
Copy link
Author

Just tagging previous reviewers in the hopes someone can take a look @denyeart @yacovm

@yacovm
Copy link
Contributor

yacovm commented Jun 28, 2023

Just tagging previous reviewers in the hopes someone can take a look @denyeart @yacovm

I don't have merge rights for this repository though.look at: https://github.com/hyperledger/fabric-samples/blob/main/MAINTAINERS.md

@robevansuk
Copy link
Author

Tagging maintainers @jkneubuh @mbwhite @nikhil550 @satota2 for a review. This Pr defers deleting token balances until after the necessary funds have been allocated. Without it, token balances are deleted before the necessary funds are identified. When testing in isolation this lead to unexpected results where the token balances were not as expected for failed transactions.

Copy link
Contributor

@satota2 satota2 left a comment

Choose a reason for hiding this comment

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

@robevansuk Thank you for your contribution.
Maybe it's my lack of understanding, but since the deletions occurred within a single transaction, token loss did not seem to occur upon failure.

Would it be possible for you to provide a specific example of a case where this would cause a problem?

@robevansuk
Copy link
Author

robevansuk commented Jul 4, 2023

I think you may be right - if the transaction fails, the token loss wouldn't occur. However testing the function in isolation does highlight that the deletion occurs before the required token balances for the transaction to occur have been earmarked. This means testing in isolation doesn't work as expected since the data is deleted, then an error occurs (when not enough tokens are located), and the state becomes inconsistent with what is expected. So ultimately this change improves the code enough to make it worth while (imo).

Deferring the deletion until after the check for partialbalance > necessaryTokens works better from a logical point of view. Deleting the token balances, then erroring out is not testable, as it was previously. I have tests for my own version of this (which is how I came across this issue). This update would result in a less surprising mechanism, whereby tokens are only removed if enough are located for the transaction to occur.

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants