Fixes for fungible-token

[vivekvpandya]

- Add event "TransferFrom" to be used as reply to TransferFrom message instead of sending two separate events.
- Replace H256 uses with ActorId.
- Use msg::source() instead of taking `source` in message payload.
- Add new messages/reply in fungible-token named "AddAdmin/RemoveAdmin".
- Admin can also mint/burn token in addition to token creator.
- Updates in test YAML as per above changes.

Partially Resolves #11 @shamilsan @NikVolf @dkubatko @LouiseMedova @gshep @breathx

[LouiseMedova]

I think it's necessary to add tests for errors. You should also probably add reading of the contract state off-chain

[vivekvpandya]

I think it's necessary to add tests for errors. You should also probably add reading of the contract state off-chain

Is it okay to do that as separate PR? As I also want to create curve-amm PR which needs these changes.

[LouiseMedova]

1)What are functions name , symbol, decimals and total_supply for? 2)Again in transfer function: I can neither be a sender nor have allowance to transfer the sender's tokens, but I can call that function and transfer tokens from sender to recipient. That is not correct work of ERC20 3) It is possible to make one action transfer instead of 2 actions transfer and transferFrom 4) Functions increase_total_supply, decrease_total_supply, set_balance, get_balance, balance_of contain only one line and make the contract bigger, would it be better to do without them?

[vivekvpandya]

1)What are functions 'name' , 'symbol', 'decimals' and 'total_supply' for? 2)Again in transfer function: I can neither be a sender nor have allowance to transfer the sender's tokens, but I can call that function and transfer tokens from sender to recipient. That is not correct work of ERC20

transfer and transfer_from both (infact all functions) are private functions so any other library/code can not call them. only way to invoke them is to send appropriate Action message, and processing on both message uses msg::source() appropriately. So I think it is correct as per https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1051db38025e8b0820da48e7f69724170ff6d552/contracts/token/ERC20/ERC20.sol#L229 where _transfer() is private function and used by other functions where those users are expected to specify correct value for sender argument.

if my above understanding is incorrect , could you please provide example scenario showing issue?

[LouiseMedova]

transfer and transfer_from both (in fact all functions) are private functions so any other library/code can not call them.

Sorry you check it in actions, that is correct now

[vivekvpandya]

transfer and transfer_from both (in fact all functions) are private functions so any other library/code can not call them.

No, that is a contract and I can call it from any address with any input arguments. And for example you call them from curve-amm.

Only way to interact with FungibleToken smart contract is to send Action messages. In updated Curve-Amm code I use transfer when pool itself wants to send token from its balance. For other's account it uses TransferFrom

[LouiseMedova]

Only way to interact with FungibleToken smart contract is to send Action messages. In updated Curve-Amm code I use transfer when pool itself wants to send token from its balance. For other's account it uses TransferFrom

Sorry, I was not right, I didn't see the msg::source() in Action

[vivekvpandya]

3) It is possible to make one action transfer instead of 2 actions transfer and transferFrom Is there any issue with current approach of having 2 separate actions? 4) In function transferFrom you send 2 replies calling approve and transfer functions but you should send one reply. It sends only one reply https://github.com/gear-tech/apps/pull/13/files#diff-67c86dec9ca66f41d2250b503b54f5b2b9ca1b139f3e8d8f235ace08b90ccc63R268 5) Functions increase_total_supply, decrease_total_supply, set_balance, get_balance, balance_of contain only one line and make the contract bigger, would it be better to do without them? what is the limit in terms of binary size? comparing to real world smart contracts this is not big.

[LouiseMedova]

Is there any issue with current approach of having 2 separate actions? what is the limit in terms of binary size? comparing to real world smart contracts this is not big.

There are no issues with current approach and no limit, I just think it would be visually more compact and readable but of course that is up to you

It sends only one reply

Yeah, again I missed that now it in actions

[vivekvpandya]

1. Please add state querying functionality to this contract. Functions like decimals, balance_of, etc. (https://ethereum.org/en/developers/docs/standards/tokens/erc-20/#methods) should be available for usage via querying. I believe you added the implementations for the functions themselves, however they just hang around as dead code at this point.

No planning to add state querying in this PR. if we don't follow incremental model nothing gets committed which is not good.

2. Please remove redundant functions like increase_total_supply, decrease_total_supply, etc. as there is no reason for their existence. Such functions do make sense in cases where there is a need for error checking or private code access, however it is not the case in this specific implementation. Please read https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1051db38025e8b0820da48e7f69724170ff6d552/contracts/token/ERC20/ERC20.sol#L186 increase/decrease_total_supply are there. Unless binary code side is an issue, I don't want to remove useful functions which are dead code at the moment.

[LouiseMedova]

1. Please add state querying functionality to this contract. Functions like decimals, balance_of, etc. (https://ethereum.org/en/developers/docs/standards/tokens/erc-20/#methods) should be available for usage via querying. I believe you added the implementations for the functions themselves, however they just hang around as dead code at this point.

No planning to add state querying in this PR. if we don't follow incremental model nothing gets committed which is not good.

2. Please remove redundant functions like `increase_total_supply`, `decrease_total_supply`, etc. as there is no reason for their existence. Such functions do make sense in cases where there is a need for error checking or private code access, however it is not the case in this specific implementation.

I agree with Daniil, moreover we should not copy a contract implemented on another blockchain. That's an ethereum implementation, and functions name, balanceOf, decimals are view functions and make sense there. They are not needed here.

[LouiseMedova]

Suggested things are big change and can be done incrementally. We can't block well reviwed code for that.

It is not big change, it is just adding the function metastate. And I still think that tests should be completely written.

[vivekvpandya]

Please do not dismiss requested changes for a PR unless the conversation is resolved. Thank you. Then how one can make progress? A developer can't spend time to chase all reviewers to get their approval. I request github admins to disable dismiss approval if that is not expected.

1. It makes more sense to merge completed pieces of code via PRs. If you'd like to split the ERC-20 implementation into two PRs (w/o the state querying and with state querying), please clean this one up from dead code that would be used in the future. You can add those helper functions later in the second PR. In practice people do have small changes/code which are not directly related to subject of PR. Also if things are already there then why to delete it and then bring it back?

2. Existence of those functions in an arbitrary Ethereum library does not justify their presence in this PR. Those might be necessary for interface implementation in Solidity, but we are working with gstd. Please remember that essential protocol implementations like ERC-20 should serve as a reference for the community. On a side note, gas fees do depend on the size of the code uploaded and amount of wasm operations executed, therefore, technically, having those functions is more expensive than not having them. I don't see any reason to keep them in this implementation. I was asked to implement https://eips.ethereum.org/EIPS/eip-20 which is standard/protocol so I though it is good to cover all possible things as some user may be looking for that. I agree on code size but could you please provide a solid numbers (or teach me to find them). Usually some % tolerance is accepted. For wasm operation executed, dead code does not matter as that does not contribute to number of wasm instructions executed.

@AndrePanin could you clarify if this is going to be used as an example or more like a production quality library? if later is recommended then I can spend time ASAP and create PR to added more tests.

[AndrePanin]

Please do not dismiss requested changes for a PR unless the conversation is resolved. Thank you. Then how one can make progress? A developer can't spend time to chase all reviewers to get their approval. I request github admins to disable dismiss approval if that is not expected.

1. It makes more sense to merge completed pieces of code via PRs. If you'd like to split the ERC-20 implementation into two PRs (w/o the state querying and with state querying), please clean this one up from dead code that would be used in the future. You can add those helper functions later in the second PR. In practice people do have small changes/code which are not directly related to subject of PR. Also if things are already there then why to delete it and then bring it back?

2. Existence of those functions in an arbitrary Ethereum library does not justify their presence in this PR. Those might be necessary for interface implementation in Solidity, but we are working with gstd. Please remember that essential protocol implementations like ERC-20 should serve as a reference for the community. On a side note, gas fees do depend on the size of the code uploaded and amount of wasm operations executed, therefore, technically, having those functions is more expensive than not having them. I don't see any reason to keep them in this implementation.

I was asked to implement https://eips.ethereum.org/EIPS/eip-20 which is standard/protocol so I though it is good to cover all possible things as some user may be looking for that. I agree on code size but could you please provide a solid numbers (or teach me to find them). Usually some % tolerance is accepted. For wasm operation executed, dead code does not matter as that does not contribute to number of wasm instructions executed.

@AndrePanin could you clarify if this is going to be used as an example or more like a production quality library? if later is recommended then I can spend time ASAP and create PR to added more tests.

It is going to be a reference example of writing a smart contract on Gear. It should demonstrate the most efficient code example for Gear contract developers. Today we discussed that this PR can unblock Chinese workshop. If tests are not ready yet, I suppose it is ok to move test writing to another PR. But I think it is as an exception. Generally code and tests should be in the same PR.

[dkubatko]

Please do not dismiss requested changes for a PR unless the conversation is resolved. Thank you. Then how one can make progress? A developer can't spend time to chase all reviewers to get their approval. I request github admins to disable dismiss approval if that is not expected.

1. It makes more sense to merge completed pieces of code via PRs. If you'd like to split the ERC-20 implementation into two PRs (w/o the state querying and with state querying), please clean this one up from dead code that would be used in the future. You can add those helper functions later in the second PR. In practice people do have small changes/code which are not directly related to subject of PR. Also if things are already there then why to delete it and then bring it back?

2. Existence of those functions in an arbitrary Ethereum library does not justify their presence in this PR. Those might be necessary for interface implementation in Solidity, but we are working with gstd. Please remember that essential protocol implementations like ERC-20 should serve as a reference for the community. On a side note, gas fees do depend on the size of the code uploaded and amount of wasm operations executed, therefore, technically, having those functions is more expensive than not having them. I don't see any reason to keep them in this implementation.

I was asked to implement https://eips.ethereum.org/EIPS/eip-20 which is standard/protocol so I though it is good to cover all possible things as some user may be looking for that. I agree on code size but could you please provide a solid numbers (or teach me to find them). Usually some % tolerance is accepted. For wasm operation executed, dead code does not matter as that does not contribute to number of wasm instructions executed.

@AndrePanin could you clarify if this is going to be used as an example or more like a production quality library? if later is recommended then I can spend time ASAP and create PR to added more tests.

https://eips.ethereum.org/EIPS/eip-20 is a protocol for Ethereum/Solidity which implies having state interactions (querying) by default. In regards to the balance_of, name, etc. functions, those are defined as view, so if you'd want a complete re-implementation for Gear you'd need to add metastate. As for the dead code, these functions are part of the OpenZeppelin implementation, i.e. a specific case of the ERC-20 protocol implementation, we can use it as a reference, but don't have to copy those line-by-line. As long as the functionality described in https://eips.ethereum.org/EIPS/eip-20 protocol is implemented, it should be sufficient for us.

[LouiseMedova]

@vivekvpandya In the test there are 9 messages and 8 expected messages. The test fails, because you missed one expected message in logs (it is the 8th message transferFrom).

[vivekvpandya]

@vivekvpandya In the test there are 9 messages and 8 expected messages. The test fails, because you missed one expected message in logs (it is the 8th message transferFrom).

ah :sweat_smile: , thanks! it works now.

[vivekvpandya]

Looks good, thank you for making these changes :)

Thanks for being patient and do review! :)