graphql: remove unused error-return

[joohhnnn]

remove unused error-return

[holiman]

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

[joohhnnn]

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

[s1na]

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.

[joohhnnn]

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？

[holiman]

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.

[joohhnnn]

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.

[s1na]

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

[joohhnnn]

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

[s1na]

This will change the current behavior as such: right now we return nil if a tx is not found, after this an error will be returned.

Where important we check tx == nil. Have you faced a crash? can you add a re-producing test?

[joohhnnn]

This will change the current behavior as such: right now we return nil if a tx is not found, after this an error will be returned.

Where important we check tx == nil. Have you faced a crash? can you add a re-producing test?

1. ->"Where important we check tx == nil. Have you faced a crash?"
   sorry for the confused, but I didnt faced any crash, I do this change for 9dcafab (remove unused error-return)
2. ->"can you add a re-producing test?"
   Do you mean add some test files for this modification? Or on the premise that I encountered a crash (although I did not encounter a crash), the test code to reproduce the crash

@s1na

[s1na]

The linter is failing:

>>> build/cache/golangci-lint-1.51.1-linux-amd64/golangci-lint run --config .golangci.yml ./...
beacon/types/config.go:124: File is not `goimports`-ed (goimports)
	
graphql/graphql.go:374: File is not `goimports`-ed (goimports)
	tx, _:= t.resolve(ctx)
util.go:48: exit status 1
exit status 1
Command exited with code 1

[joohhnnn]

The linter is failing:

>>> build/cache/golangci-lint-1.51.1-linux-amd64/golangci-lint run --config .golangci.yml ./...
beacon/types/config.go:124: File is not `goimports`-ed (goimports)
	
graphql/graphql.go:374: File is not `goimports`-ed (goimports)
	tx, _:= t.resolve(ctx)
util.go:48: exit status 1
exit status 1
Command exited with code 1

fixed，PLZ check again @s1na

[s1na]

LGTM