Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: include the reason when the redistribution contract call fails #4358

Merged
merged 1 commit into from
Sep 29, 2023
Merged

fix: include the reason when the redistribution contract call fails #4358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions pkg/postage/postagecontract/contract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ func (c *postageContract) sendApproveTransaction(ctx context.Context, amount *bi
return receipt, nil
}

func (c *postageContract) sendTransaction(ctx context.Context, callData []byte, desc string) (*types.Receipt, error) {
func (c *postageContract) sendTransaction(ctx context.Context, callData []byte, desc string) (receipt *types.Receipt, err error) {
request := &transaction.TxRequest{
To: &c.postageStampContractAddress,
Data: callData,
Expand All @@ -187,25 +187,22 @@ func (c *postageContract) sendTransaction(ctx context.Context, callData []byte,
Value: big.NewInt(0),
Description: desc,
}
defer func() {
err = c.transactionService.UnwrapRevertReason(ctx, request, err)
}()

txHash, err := c.transactionService.Send(ctx, request, transaction.DefaultTipBoostPercent)
if err != nil {
return nil, err
}

receipt, err := c.transactionService.WaitForReceipt(ctx, txHash)
receipt, err = c.transactionService.WaitForReceipt(ctx, txHash)
if err != nil {
return nil, err
}

if receipt.Status == 0 {
err := transaction.ErrTransactionReverted
if res, cErr := c.transactionService.Call(ctx, request); cErr == nil {
if reason, uErr := abi.UnpackRevert(res); uErr == nil {
err = fmt.Errorf("%w: reason: %s", err, reason)
}
}
return nil, err
return nil, transaction.ErrTransactionReverted
}

return receipt, nil
Expand Down
2 changes: 1 addition & 1 deletion pkg/storageincentives/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
"github.com/ethersphere/bee/pkg/storage"
"github.com/ethersphere/bee/pkg/storageincentives/redistribution"
"github.com/ethersphere/bee/pkg/storageincentives/staking"
storer "github.com/ethersphere/bee/pkg/storer"
"github.com/ethersphere/bee/pkg/storer"
"github.com/ethersphere/bee/pkg/swarm"
"github.com/ethersphere/bee/pkg/transaction"
)
Expand Down
6 changes: 4 additions & 2 deletions pkg/storageincentives/redistribution/redistribution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,10 @@ func (c *contract) ReserveSalt(ctx context.Context) ([]byte, error) {
return salt[:], nil
}

func (c *contract) sendAndWait(ctx context.Context, request *transaction.TxRequest, boostPercent int) (common.Hash, error) {
txHash, err := c.txService.Send(ctx, request, boostPercent)
func (c *contract) sendAndWait(ctx context.Context, request *transaction.TxRequest, boostPercent int) (txHash common.Hash, err error) {
defer func() { err = c.txService.UnwrapRevertReason(ctx, request, err) }()

txHash, err = c.txService.Send(ctx, request, boostPercent)
if err != nil {
return txHash, err
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/transaction/mock/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ func (m *transactionServiceMock) TransactionFee(ctx context.Context, txHash comm
return big.NewInt(0), nil
}

func (m *transactionServiceMock) UnwrapRevertReason(_ context.Context, _ *transaction.TxRequest, err error) error {
return err
}

// Option is the option passed to the mock Chequebook service
type Option interface {
apply(*transactionServiceMock)
Expand Down
18 changes: 18 additions & 0 deletions pkg/transaction/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"time"

"github.com/ethereum/go-ethereum"
"github.com/ethereum/go-ethereum/accounts/abi"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethersphere/bee/pkg/crypto"
Expand Down Expand Up @@ -94,6 +95,9 @@ type Service interface {
CancelTransaction(ctx context.Context, originalTxHash common.Hash) (common.Hash, error)
// TransactionFee retrieves the transaction fee
TransactionFee(ctx context.Context, txHash common.Hash) (*big.Int, error)
// UnwrapRevertReason tries to unwrap the revert reason if the given error is not nil.
// The original error is wrapped in case the revert reason exists.
UnwrapRevertReason(ctx context.Context, req *TxRequest, err error) error
}

type transactionService struct {
Expand Down Expand Up @@ -584,3 +588,17 @@ func (t *transactionService) TransactionFee(ctx context.Context, txHash common.H
}
return trx.Cost(), nil
}

func (t *transactionService) UnwrapRevertReason(ctx context.Context, req *TxRequest, err error) error {
if err == nil {
return nil
}

if res, cErr := t.Call(ctx, req); cErr == nil {
if reason, uErr := abi.UnpackRevert(res); uErr == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function may benefit from unit tests with different types of revert errors to ensure that they are not silently ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is for revert reason as specified in: https://solidity.readthedocs.io/en/latest/control-structures.html#revert

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the custom errors be rewritten into this generic form so the error extraction is unified?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, merging this. The custom errors need to be reviewed and handled in different PR.

Copy link

@0xCardinalError 0xCardinalError Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

err = fmt.Errorf("%w: reason: %s", err, reason)
}
}

return err
}
Loading