fix: include the reason when the redistribution contract call fails #4358
Conversation
mrekucci
force-pushed
the
redistribution-add-revert-reason
branch
2 times, most recently
from
3f71e2a to
8c6d160
Compare
mrekucci
force-pushed
the
redistribution-add-revert-reason
branch
from
8c6d160 to
beb5631
Compare
| @@ -191,7 +191,7 @@ func (c *contract) sendAndWait(ctx context.Context, request *transaction.TxReque | |||
| } | |||
|
|
|||
| if receipt.Status == 0 { | |||
| return txHash, transaction.ErrTransactionReverted | |||
| return txHash, c.txService.UnwrapRevertReason(ctx, request, transaction.ErrTransactionReverted) | |||
There was a problem hiding this comment.
The process seems to fail before it would get a receipt. Supposedly the EstimateGas call (invoked by c.txService.Send above) returns with exection reverted.
mrekucci
force-pushed
the
redistribution-add-revert-reason
branch
from
beb5631 to
d6af522
Compare
| } | ||
| } | ||
| return nil, err | ||
| return nil, c.transactionService.UnwrapRevertReason(ctx, request, transaction.ErrTransactionReverted) |
There was a problem hiding this comment.
could be deferred at the starting of the function such as in redistribution.go
| } | ||
|
|
||
| if res, cErr := t.Call(ctx, req); cErr == nil { | ||
| if reason, uErr := abi.UnpackRevert(res); uErr == nil { |
There was a problem hiding this comment.
UnapckRevert will unpack abi encoded "Error(string)", but I do not think that it will unpack custom errors that are for example used in the redistribution contract. To unpack custom errors, contract abi is required, get the error with abi.ABI.ErrorByID using the first 4 bytes of the data and use the retuned *abi.Error to Unpack the error.
I did not try it, in this PR, but used this approach to get custom errors from the call traces.
There was a problem hiding this comment.
This function may benefit from unit tests with different types of revert errors to ensure that they are not silently ignored.
There was a problem hiding this comment.
Yes, this is for revert reason as specified in: https://solidity.readthedocs.io/en/latest/control-structures.html#revert
There was a problem hiding this comment.
Exactly, this is the one type of revert, there may be custom errors packed, and custom errors are used in our contracts. They supposed to be packed differently as they have arguments and custom name.
There was a problem hiding this comment.
Can the custom errors be rewritten into this generic form so the error extraction is unified?
There was a problem hiding this comment.
They can be just strings, but sometimes additional values are useful to be passed. In any case, solidity allows custom revert errors and I do not think that they are unpacked here.
There was a problem hiding this comment.
Ok, merging this. The custom errors need to be reviewed and handled in different PR.
There was a problem hiding this comment.
Can the custom errors be rewritten into this generic form so the error extraction is unified?
actually no, this generic or string-only error form wasted a lot of blockchain space so was turned into a less wasteful form of generating errors, which in turn saves gas and in the end money for users. It's not a big deal to use them just need to be unpacked.
mrekucci
force-pushed
the
redistribution-add-revert-reason
branch
from
d6af522 to
7044098
Compare
Checklist
Description
Indicate the reason for the unsuccessful call of the redistribution contract.