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

[robevansuk]

… 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]

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

[yacovm]

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]

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.

[robevansuk]

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.