Skip to content

graphql: remove unused error-return #27285

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

Merged
merged 5 commits into from
Jun 5, 2023
Merged

graphql: remove unused error-return #27285

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c7d1fbb
remove resolve unused error returned
joohhnnn May 16, 2023
9dcafab
Modify detail
joohhnnn May 16, 2023
202863a
handle non-existent tx error
joohhnnn May 17, 2023
5b8b661
Revert to 9dcafa...
joohhnnn May 17, 2023
c8f93f8
Added spaces
joohhnnn May 25, 2023
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
219 changes: 101 additions & 118 deletions graphql/graphql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,11 +197,11 @@ type Transaction struct {

// resolve returns the internal transaction object, fetching it if needed.
// It also returns the block the tx belongs to, unless it is a pending tx.
func (t *Transaction) resolve(ctx context.Context) (*types.Transaction, *Block, error) {
func (t *Transaction) resolve(ctx context.Context) (*types.Transaction, *Block) {
t.mu.Lock()
defer t.mu.Unlock()
if t.tx != nil {
return t.tx, t.block, nil
return t.tx, t.block
}
// Try to return an already finalized transaction
tx, blockHash, _, index, err := t.r.backend.GetTransaction(ctx, t.hash)
Expand All @@ -214,58 +214,58 @@ func (t *Transaction) resolve(ctx context.Context) (*types.Transaction, *Block,
hash: blockHash,
}
t.index = index
return t.tx, t.block, nil
return t.tx, t.block
}
// No finalized transaction, try to retrieve it from the pool
t.tx = t.r.backend.GetPoolTransaction(t.hash)
return t.tx, nil, nil
return t.tx, nil
}

func (t *Transaction) Hash(ctx context.Context) common.Hash {
return t.hash
}

func (t *Transaction) InputData(ctx context.Context) (hexutil.Bytes, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return hexutil.Bytes{}, err
func (t *Transaction) InputData(ctx context.Context) hexutil.Bytes {
tx, _ := t.resolve(ctx)
if tx == nil {
return hexutil.Bytes{}
}
return tx.Data(), nil
return tx.Data()
}

func (t *Transaction) Gas(ctx context.Context) (hexutil.Uint64, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return 0, err
func (t *Transaction) Gas(ctx context.Context) hexutil.Uint64 {
tx, _ := t.resolve(ctx)
if tx == nil {
return 0
}
return hexutil.Uint64(tx.Gas()), nil
return hexutil.Uint64(tx.Gas())
}

func (t *Transaction) GasPrice(ctx context.Context) (hexutil.Big, error) {
tx, block, err := t.resolve(ctx)
if err != nil || tx == nil {
return hexutil.Big{}, err
func (t *Transaction) GasPrice(ctx context.Context) hexutil.Big {
tx, block := t.resolve(ctx)
if tx == nil {
return hexutil.Big{}
}
switch tx.Type() {
case types.AccessListTxType:
return hexutil.Big(*tx.GasPrice()), nil
return hexutil.Big(*tx.GasPrice())
case types.DynamicFeeTxType:
if block != nil {
if baseFee, _ := block.BaseFeePerGas(ctx); baseFee != nil {
// price = min(tip, gasFeeCap - baseFee) + baseFee
return (hexutil.Big)(*math.BigMin(new(big.Int).Add(tx.GasTipCap(), baseFee.ToInt()), tx.GasFeeCap())), nil
return (hexutil.Big)(*math.BigMin(new(big.Int).Add(tx.GasTipCap(), baseFee.ToInt()), tx.GasFeeCap()))
}
}
return hexutil.Big(*tx.GasPrice()), nil
return hexutil.Big(*tx.GasPrice())
default:
return hexutil.Big(*tx.GasPrice()), nil
return hexutil.Big(*tx.GasPrice())
}
}

func (t *Transaction) EffectiveGasPrice(ctx context.Context) (*hexutil.Big, error) {
tx, block, err := t.resolve(ctx)
if err != nil || tx == nil {
return nil, err
tx, block := t.resolve(ctx)
if tx == nil {
return nil, nil
}
// Pending tx
if block == nil {
Expand All @@ -281,40 +281,40 @@ func (t *Transaction) EffectiveGasPrice(ctx context.Context) (*hexutil.Big, erro
return (*hexutil.Big)(math.BigMin(new(big.Int).Add(tx.GasTipCap(), header.BaseFee), tx.GasFeeCap())), nil
}

func (t *Transaction) MaxFeePerGas(ctx context.Context) (*hexutil.Big, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return nil, err
func (t *Transaction) MaxFeePerGas(ctx context.Context) *hexutil.Big {
tx, _ := t.resolve(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong. The resolve on a transaction ought to be able to return an error, I think. The fact that it currently does not looks like a flaw to me.

	tx, blockHash, _, index, err := t.r.backend.GetTransaction(ctx, t.hash)
	if err == nil && tx != nil {
             ...
	}
	// No finalized transaction, try to retrieve it from the pool
	t.tx = t.r.backend.GetPoolTransaction(t.hash)
	return t.tx, nil, nil
}

We just "forget" the err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense, Try to add an error handling that can't find tx in txpool now @holiman

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong. The resolve on a transaction ought to be able to return an error, I think. The fact that it currently does not looks like a flaw to me.

The idea here is that we try to resolve and return nil to user if no such tx was found. This is similar behavior to eth.getTransactionByHash.

So IMO the initial PR that removed the error param is ok. Only we can add a comment to clarify this point. And make sure nowhere we can trigger a panic on nil.

Copy link
Contributor Author

@joohhnnn joohhnnn May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This seems wrong. The resolve on a transaction ought to be able to return an error, I think. The fact that it currently does not looks like a flaw to me.

The idea here is that we try to resolve and return nil to user if no such tx was found. This is similar behavior to eth.getTransactionByHash.

So IMO the initial PR that removed the error param is ok. Only we can add a comment to clarify this point. And make sure nowhere we can trigger a panic on nil.

Already revert commit. BTW getTransactionByHash() also has this problem. HeaderByhash() and GetTransaction() in eth/api_backend.go also always return a nil as the return value of error. To be precise, there are many such problems in api_backend, such as 'getpoolTransactions()'. Should I open a new PR to modify api_backend.go？

Copy link
Contributor

@holiman holiman May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The idea here is that we try to resolve and return nil to user if no such tx was found. This is similar behavior to eth.getTransactionByHash.

That might be fine for a lot of cases, such as this right here. But IMO the case where this fails are these:

func (t *Transaction) Nonce(ctx context.Context) hexutil.Uint64 
func (t *Transaction) InputData(ctx context.Context) hexutil.Bytes 
func (t *Transaction) Gas(ctx context.Context) hexutil.Uint64

So the Nonce will return 0 if the transaction cannot be found. It cannot return nil, to signal "could not find it", and if we don't have the ability to return an error, we'll just return a wrong answer. Might be only Nonce, Gas and InputData that are affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is that we try to resolve and return nil to user if no such tx was found. This is similar behavior to eth.getTransactionByHash.

That might be fine for a lot of cases, such as this right here. But IMO the case where this fails are these:


func (t *Transaction) Nonce(ctx context.Context) hexutil.Uint64 

func (t *Transaction) InputData(ctx context.Context) hexutil.Bytes 

func (t *Transaction) Gas(ctx context.Context) hexutil.Uint64

So the Nonce will return 0 if the transaction cannot be found. It cannot return nil, to signal "could not find it", and if we don't have the ability to return an error, we'll just return a wrong answer. Might be only Nonce, Gas and InputData that are affected.

@holiman l think I understand what you mean now. Even though we have addressed the issue of returning nil error, we should also pay attention to the problem of returning 0 when certain data like nonce is not found in a transaction (which can be misleading as if nonce is found to be 0). Instead, we should provide a prompt indicating the failure to find the requested data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I thought I responded yesterday. That shouldn't be a problem. Because if a tx doesn't exist we will not have a Transaction object in the response anyway. Here are the various ways you could query a tx within the schema:

  • Query directly by hash -> returns nil if non-existent
  • Get transactions of a block -> implies tx exists
  • Get pending txes -> implies tx exists
  • Given a Log, get transaction that emitted it -> implies tx exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I thought I responded yesterday. That shouldn't be a problem. Because if a tx doesn't exist we will not have a Transaction object in the response anyway. Here are the various ways you could query a tx within the schema:

  • Query directly by hash -> returns nil if non-existent
  • Get transactions of a block -> implies tx exists
  • Get pending txes -> implies tx exists
  • Given a Log, get transaction that emitted it -> implies tx exists

so this version is still cool ? c8f93f8

if tx == nil {
return nil
}
switch tx.Type() {
case types.AccessListTxType:
return nil, nil
return nil
case types.DynamicFeeTxType:
return (*hexutil.Big)(tx.GasFeeCap()), nil
return (*hexutil.Big)(tx.GasFeeCap())
default:
return nil, nil
return nil
}
}

func (t *Transaction) MaxPriorityFeePerGas(ctx context.Context) (*hexutil.Big, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return nil, err
func (t *Transaction) MaxPriorityFeePerGas(ctx context.Context) *hexutil.Big {
tx, _ := t.resolve(ctx)
if tx == nil {
return nil
}
switch tx.Type() {
case types.AccessListTxType:
return nil, nil
return nil
case types.DynamicFeeTxType:
return (*hexutil.Big)(tx.GasTipCap()), nil
return (*hexutil.Big)(tx.GasTipCap())
default:
return nil, nil
return nil
}
}

func (t *Transaction) EffectiveTip(ctx context.Context) (*hexutil.Big, error) {
tx, block, err := t.resolve(ctx)
if err != nil || tx == nil {
return nil, err
tx, block := t.resolve(ctx)
if tx == nil {
return nil, nil
}
// Pending tx
if block == nil {
Expand All @@ -336,81 +336,72 @@ func (t *Transaction) EffectiveTip(ctx context.Context) (*hexutil.Big, error) {
}

func (t *Transaction) Value(ctx context.Context) (hexutil.Big, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return hexutil.Big{}, err
tx, _ := t.resolve(ctx)
if tx == nil {
return hexutil.Big{}, nil
}
if tx.Value() == nil {
return hexutil.Big{}, fmt.Errorf("invalid transaction value %x", t.hash)
}
return hexutil.Big(*tx.Value()), nil
}

func (t *Transaction) Nonce(ctx context.Context) (hexutil.Uint64, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return 0, err
func (t *Transaction) Nonce(ctx context.Context) hexutil.Uint64 {
tx, _ := t.resolve(ctx)
if tx == nil {
return 0
}
return hexutil.Uint64(tx.Nonce()), nil
return hexutil.Uint64(tx.Nonce())
}

func (t *Transaction) To(ctx context.Context, args BlockNumberArgs) (*Account, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return nil, err
func (t *Transaction) To(ctx context.Context, args BlockNumberArgs) *Account {
tx, _ := t.resolve(ctx)
if tx == nil {
return nil
}
to := tx.To()
if to == nil {
return nil, nil
return nil
}
return &Account{
r: t.r,
address: *to,
blockNrOrHash: args.NumberOrLatest(),
}, nil
}
}

func (t *Transaction) From(ctx context.Context, args BlockNumberArgs) (*Account, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return nil, err
func (t *Transaction) From(ctx context.Context, args BlockNumberArgs) *Account {
tx, _ := t.resolve(ctx)
if tx == nil {
return nil
}
signer := types.LatestSigner(t.r.backend.ChainConfig())
from, _ := types.Sender(signer, tx)
return &Account{
r: t.r,
address: from,
blockNrOrHash: args.NumberOrLatest(),
}, nil
}
}

func (t *Transaction) Block(ctx context.Context) (*Block, error) {
_, block, err := t.resolve(ctx)
if err != nil {
return nil, err
}
return block, nil
func (t *Transaction) Block(ctx context.Context) *Block {
_, block := t.resolve(ctx)
return block
}

func (t *Transaction) Index(ctx context.Context) (*hexutil.Uint64, error) {
_, block, err := t.resolve(ctx)
if err != nil {
return nil, err
}
func (t *Transaction) Index(ctx context.Context) *hexutil.Uint64 {
_, block := t.resolve(ctx)
// Pending tx
if block == nil {
return nil, nil
return nil
}
index := hexutil.Uint64(t.index)
return &index, nil
return &index
}

// getReceipt returns the receipt associated with this transaction, if any.
func (t *Transaction) getReceipt(ctx context.Context) (*types.Receipt, error) {
_, block, err := t.resolve(ctx)
if err != nil {
return nil, err
}
_, block := t.resolve(ctx)
// Pending tx
if block == nil {
return nil, nil
Expand Down Expand Up @@ -465,10 +456,7 @@ func (t *Transaction) CreatedContract(ctx context.Context, args BlockNumberArgs)
}

func (t *Transaction) Logs(ctx context.Context) (*[]*Log, error) {
_, block, err := t.resolve(ctx)
if err != nil {
return nil, err
}
_, block := t.resolve(ctx)
// Pending tx
if block == nil {
return nil, nil
Expand Down Expand Up @@ -504,19 +492,16 @@ func (t *Transaction) getLogs(ctx context.Context, hash common.Hash) (*[]*Log, e
return &ret, nil
}

func (t *Transaction) Type(ctx context.Context) (*hexutil.Uint64, error) {
tx, _, err := t.resolve(ctx)
if err != nil {
return nil, err
}
func (t *Transaction) Type(ctx context.Context) *hexutil.Uint64 {
tx, _ := t.resolve(ctx)
txType := hexutil.Uint64(tx.Type())
return &txType, nil
return &txType
}

func (t *Transaction) AccessList(ctx context.Context) (*[]*AccessTuple, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return nil, err
func (t *Transaction) AccessList(ctx context.Context) *[]*AccessTuple {
tx, _ := t.resolve(ctx)
if tx == nil {
return nil
}
accessList := tx.AccessList()
ret := make([]*AccessTuple, 0, len(accessList))
Expand All @@ -526,40 +511,40 @@ func (t *Transaction) AccessList(ctx context.Context) (*[]*AccessTuple, error) {
storageKeys: al.StorageKeys,
})
}
return &ret, nil
return &ret
}

func (t *Transaction) R(ctx context.Context) (hexutil.Big, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return hexutil.Big{}, err
func (t *Transaction) R(ctx context.Context) hexutil.Big {
tx, _ := t.resolve(ctx)
if tx == nil {
return hexutil.Big{}
}
_, r, _ := tx.RawSignatureValues()
return hexutil.Big(*r), nil
return hexutil.Big(*r)
}

func (t *Transaction) S(ctx context.Context) (hexutil.Big, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return hexutil.Big{}, err
func (t *Transaction) S(ctx context.Context) hexutil.Big {
tx, _ := t.resolve(ctx)
if tx == nil {
return hexutil.Big{}
}
_, _, s := tx.RawSignatureValues()
return hexutil.Big(*s), nil
return hexutil.Big(*s)
}

func (t *Transaction) V(ctx context.Context) (hexutil.Big, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return hexutil.Big{}, err
func (t *Transaction) V(ctx context.Context) hexutil.Big {
tx, _ := t.resolve(ctx)
if tx == nil {
return hexutil.Big{}
}
v, _, _ := tx.RawSignatureValues()
return hexutil.Big(*v), nil
return hexutil.Big(*v)
}

func (t *Transaction) Raw(ctx context.Context) (hexutil.Bytes, error) {
tx, _, err := t.resolve(ctx)
if err != nil || tx == nil {
return hexutil.Bytes{}, err
tx, _ := t.resolve(ctx)
if tx == nil {
return hexutil.Bytes{}, nil
}
return tx.MarshalBinary()
}
Expand Down Expand Up @@ -1233,19 +1218,17 @@ func (r *Resolver) Pending(ctx context.Context) *Pending {
return &Pending{r}
}

func (r *Resolver) Transaction(ctx context.Context, args struct{ Hash common.Hash }) (*Transaction, error) {
func (r *Resolver) Transaction(ctx context.Context, args struct{ Hash common.Hash }) *Transaction {
tx := &Transaction{
r: r,
hash: args.Hash,
}
// Resolve the transaction; if it doesn't exist, return nil.
t, _, err := tx.resolve(ctx)
if err != nil {
return nil, err
} else if t == nil {
return nil, nil
t, _ := tx.resolve(ctx)
if t == nil {
return nil
}
return tx, nil
return tx
}

func (r *Resolver) SendRawTransaction(ctx context.Context, args struct{ Data hexutil.Bytes }) (common.Hash, error) {
Expand Down