ERC20 realization update for javascript #1313
Conversation
…tID, BalanceOf etc; Added function ClientAccountMSPID; Added function CheckAuthorization; Using new function _update for minting, burning and transfering tokens; Additional functions for approving, spending allowance and checking empty values; Updated and repaired tests Signed-off-by: XZSt4nce <xzstnc@mail.ru>
Deleted redundant `totalSupply` param from `Initialize` JSDoc Signed-off-by: Maxim Tiron <xzstnc@mail.ru>
|
? |
bestbeforetoday
left a comment
bestbeforetoday
left a comment
There was a problem hiding this comment.
Thank you for the contribution. Improvements to the JSDoc are great. It looks like there are some subtle differences in error handling behaviour but nothing jumped out at me as breaking the intended functionality. I have added a few comments that generally I think are worth addressing.
It would give me much more confidence in accepting changes to these samples if some basic testing of them was added to the build, similar to the ones driven by .github/workflows/test-network-*.yaml.
| * @param {Context} ctx the transaction context | ||
| * @returns {String} Returns the account MSP id | ||
| */ | ||
| ClientAccountMSPID(ctx) { |
There was a problem hiding this comment.
There is only one call to this function within the smart contract. Perhaps that one call should just be replaced with ctx.clientIdentity.getMSPID() and this function removed. The capitalised function name suggests it is intended to be evaluated by clients but this seems redundant since the client must already know its MSP ID in order to interact with the smart contract.
| // Check if the sender has enough tokens to spend. | ||
| if (fromCurrentBalance < valueInt) { | ||
| throw new Error(`client account ${from} has insufficient funds.`); | ||
| if (this.isEmpty(from)) { |
There was a problem hiding this comment.
For strings !from also covers the case where from.length === 0 so this is equivalent to the (perhaps simpler) if (!from).
| if (this.isEmpty(from)) { | |
| if (!from) { |
| toCurrentBalance = 0; | ||
| } else { | ||
| toCurrentBalance = parseInt(toCurrentBalanceBytes.toString()); | ||
| if (this.isEmpty(to)) { |
There was a problem hiding this comment.
| if (this.isEmpty(to)) { | |
| if (!to) { |
| * @param {Boolean} emitEvent Whether to emit event | ||
| */ | ||
| async _approveEvent(ctx, owner, spender, value, emitEvent) { | ||
| if (this.isEmpty(owner)) { |
There was a problem hiding this comment.
| if (this.isEmpty(owner)) { | |
| if (!owner) { |
| if (this.isEmpty(owner)) { | ||
| throw new Error('invalid approver'); | ||
| } | ||
| if (this.isEmpty(spender)) { |
There was a problem hiding this comment.
| if (this.isEmpty(spender)) { | |
| if (!spender) { |
| * @param {String} to The recipient | ||
| * @param {Number} value The amount of token to be transferred | ||
| */ | ||
| async _update(ctx, from, to, value) { |
There was a problem hiding this comment.
I appreciate that you are trying to reduce code duplication across transaction functions but I am concerned that the logic of this _update implementation introduces a lot of complexity. Some flow explicitly require from and to values while others rely on those values being falsy. This means that this function has four potential modes of invocation:
fromandtoboth set.fromset andtounset.fromunset andtoset.fromandtoboth unset.
This creates high cyclomatic complexity. I think it would be preferable if each (valid) combination was implemented separately with each combination having only the parameters actually needed to make the intent very clear, despite some similarities between them. Alternatively you could still avoid duplication by splitting specific parts of this update flow into separate functions and having transfer, mint and burn compose those parts in the appropriate combination.
| sinon.assert.calledWith(mockStub.putState.getCall(4), 'balance_Alice', Buffer.from('0')); | ||
| sinon.assert.calledWith(mockStub.putState.getCall(5), 'balance_Bob', Buffer.from('1000')); |
There was a problem hiding this comment.
The need to change the call number highlights the fragility of this testing approach. The call number should not matter. I suggest this instead, which checks that a call was made with the required arguments regardless of the call ordering:
| sinon.assert.calledWith(mockStub.putState.getCall(4), 'balance_Alice', Buffer.from('0')); | |
| sinon.assert.calledWith(mockStub.putState.getCall(5), 'balance_Bob', Buffer.from('1000')); | |
| sinon.assert.calledWith(mockStub.putState, 'balance_Alice', Buffer.from('0')); | |
| sinon.assert.calledWith(mockStub.putState, 'balance_Bob', Buffer.from('1000')); |
Same suggestion for all the other changes in this file that required a different call number.
2 participants
totalSupplyparam fromInitializeJSDocClientAccountID,BalanceOfetcClientAccountMSPIDCheckAuthorization_updatefor minting, burning and transfering tokens