search

trezor / blockbook

:blue_book: Trezor address/account balance backend
https://trezor.io
GNU Affero General Public License v3.0
647 stars 644 forks source link

Unnecessary check logic in get transaction API #1008

Closed johnsonco closed 8 months ago

johnsonco commented 8 months ago

https://github.com/trezor/blockbook/blob/master/db/txcache.go#L70

Hi team, I believe that the condition if tx.BlockHeight > 0 in this line is unnecessary. BlockHeight will always be 0 because RPC getrawtransaction doesn't return block height information; it only returns the block hash.

Could you please double-check?

image
martinboehm commented 8 months ago

Thank you for your comment. I think that you are forgetting that Blockbook serves many coins and some of them return tx.BlockHeight from GetTransaction.

johnsonco commented 8 months ago

Could you please list some coins that return tx.BlockHeight from GetTransaction? To the experiences I had, these coins forked from BTC rarely update this part of source code.

martinboehm commented 8 months ago

The point is that the common code should handle a potential case that the coin specific code returns the BlockHeight (because it can).

johnsonco commented 8 months ago

I don't agree with your logic. If none of blockbook supported nodes return BlockHeight, then this line of code serves no purpose at all; it just adds interference.

Imagine if there's an issue with this API, everyone will get stuck here, wasting time. I was indeed disrupted by this line of code and ended up spending an extra hour because of it.

johnsonco commented 8 months ago

All codes should have a reason to exist; if not, it should be removed