roman-khimov
commented
1 year ago I'd say the first one is not technically a bug, but still it's something that can be improved (CreateTransferTxAsync is method-specific, so it knows the API it uses and ASSERT will make it a bit easier to use). At least, that's the way similar NeoGo APIs work, producing transactions with ASSERT added (nep17's TokenWriter.Transfer).
The second one looks like a bug to me because MakeTransactionAsync should fail for FAULTed test invocations by default. There may be some way to override this behavior (when the user knows better), but by default most users want their transactions to end up in HALT, so it should be checked for. Again, that's the way similar NeoGo APIs work (Actor.MakeCall).
Summary or problem description I have the following issue. When we run a transaction for a certain Nep17 token, which sends tokens from one wallet to another, but the amount is more than the current balance, the transaction does not return an error. A transaction with the returned hash is successfully stored inside a block, but it actually do not transfer any tokens (because the Transfer method of the token contract returned false). When I get the state of the transaction through RPC call, it returns same data as a successful one.
Do you have any solution you want to propose?
There are 2 things which I found during this research and think they are problems which should be resolved:
CreateTransferTxAsync in NEP17Api.cs creates a script for "transfer" method, which does not add ASSERT at the end (in order to check the boolean result from Transfer method of the token contract). The implementation of MakeTransaction in Wallet.cs in Neo project does it this way, so I'd trust its implementation more and I think it is just a bug in the code of the RpcClient module.
The next strange step is TransactionManagerFactory -> MakeTransactionAsync. It does a test invoke of the transaction script, but totally ignores the result from it. In my case, if I add the ASSERT from 1), the invocation result here is FAULT. I think that this method should check that before creating a transaction and returning it. Probably throw an exception or return a result of (RpcInvokeResult, Transaction). This way the code that calls MakeTransactionAsync can decide whether to send the raw transaction or return an error or something like that. In my case if I add the ASSERT and still execute the SendRawTransaction, it also ignores the FAULT code and stores the "invalid" transfer transaction in a block successfully.
What do you think about these issues? Are they things that has to be fixed or there is a reason to create the script without ASSERT and not checking the test-invoke state before sending the raw transaction ?