Allow null TransactionReceipt per documentation #4841

wbt · 2022-03-12T01:17:10Z

Description

Documentation states that the transaction receipt could be null if no receipt was found.

An alternative would be to change this line to add | null just before >; but the proposed solution is more general as the issue may be in other places.

Type of change

Bug fix (hopefully-non-breaking change which fixes an issue) This brings code behavior more in line with what its documented as supposed to be doing. Folks using TypeScript might have to do some updates to avoid compile-time errors when they would instead currently get the type-based runtime errors TypeScript aims to prevent. This does currently break other types in Web3 as a result; those should be fixed before this is merged but the PR is made as a contribution to start community improvement.

Checklist:

coveralls · 2022-03-12T01:31:15Z

Pull Request Test Coverage Report for Build 2005946998

0 of 0 changed or added relevant lines in 0 files are covered.
429 unchanged lines in 7 files lost coverage.
Overall coverage decreased (-2.3%) to 72.21%

Files with Coverage Reduction	New Missed Lines	%
packages/web3-core-requestmanager/src/jsonrpc.js	1	88.0%
packages/web3-core-helpers/src/formatters.js	25	81.07%
packages/web3-core-helpers/src/errors.js	31	4.41%
packages/web3-utils/src/soliditySha3.js	55	5.13%
packages/web3-utils/src/index.js	62	29.31%
packages/web3-utils/src/utils.js	92	12.86%
packages/web3-eth-accounts/src/index.js	163	23.77%

Totals
Change from base Build 2002669572:	-2.3%
Covered Lines:	3368
Relevant Lines:	4396

💛 - Coveralls

wbt · 2022-03-12T01:31:38Z

When running dtslint locally (as well as in that failing check), I'm getting some errors in packages/web3-eth-ens/types/tests/ens-test.ts:48:1 of the form:

ERROR: 48:1 expect TypeScript@local expected type to be:
PromiEvent<TransactionReceipt | TransactionRevertInstructionError>
got:
PromiEvent<{ status: boolean; transactionHash: string; transactionIndex: number; blockHash: string; blockNumber: number; from: string; to: string; contractAddress?: string | undefined; cumulativeGasUsed: number; gasUsed: number; effectiveGasPrice: number; logs: Log[]; logsBloom: string; events?: { [eventName: string]: EventLog; } | undefined; } | TransactionRevertInstructionError | null>

The first and last of those, combined with the or pipe, is TransactionReceipt, so this should be a match, but it seems like either the TransactionReceipt type is not getting correctly imported or the types are getting rearranged in the expectation and this rearrangement is what's causing the issue - a problem in the test framework rather than the code change. Unfortunately I can't quite figure out which with the remaining time today. However, swapping the order of the two types in the expectation statement doesn't fix it.

wbt · 2022-03-14T16:59:02Z

On further investigation trying to simplify the issue, it appears that the types referred to in $ExpectType lines come from a completely different source than the imports at the top of the file. The imports and the tests associated with them can be completely removed and it's still getting those expected type definitions from somewhere, other than the types defined in the current repository. More specifically, it's pulling those types from the node_modules/web3-core folder inside the web3-eth-ens directory. This may be related to the pre-existing circular dependency warning

lerna WARN ECYCLE Dependency cycles detected, you should fix these!
lerna WARN ECYCLE web3-eth-ens -> web3-eth -> web3-eth-ens

It appears this may be getting resolved by having circularity broken in a way that makes it impossible to update types without having tests fail because the type being tested to in the current repo differs from the type as published on NPM.

I can see in web3-eth-ens/package.json that web3-eth is a devDependency, probably because the type definition for the Ens constructor takes an Eth object imported from web3-eth. That eth object is stored and used for a call to getBlock() (to see if it was more than an hour ago) and net.getNetworkType() (to find the address on that network) in checkNetwork().

I can also see that web3-eth-ens is a dependency of web3-eth because setProvider() updates a contained ens object to null out the detected address and last sync check. It's not clear to me what this contained ens object is used for or why it's there, and I suspect the circularity might be resolvable by removing the ENS references in web3-eth. However, I don't think that should have to be within the scope of this PR.

nazarhussain · 2022-03-17T10:42:39Z

@wbt Please resolve the conflicts for CHANGELOG.md

wbt · 2022-03-17T21:33:55Z

I did so by moving this under the list for 1.7.2. However, it's hard to predict that part as it depends on how long reviews take, especially if getting this reviewed depends on some solution to #4845 being developed first to be able to get CI input from current code instead of an npm cache.

jdevcs

@wbt Build / lint is failing with:
/packages/web3-eth-ens/types/tests/ens-test.ts:48:1 ERROR: 48:1 expect TypeScript@local expected type to be: PromiEvent<TransactionReceipt | TransactionRevertInstructionError> got: PromiEvent<{ status: boolean; transactionHash: string; transactionIndex: number; blockHash: string; blockNumber: number; from: string; to: string; contractAddress?: string | undefined; cumulativeGasUsed: number; gasUsed: number; effectiveGasPrice: number; logs: Log[]; logsBloom: string; events?: { [eventName: string]: EventLog; } | undefined; } | TransactionRevertInstructionError | null> ERROR: 50:1 expect TypeScript@local expected type to be: PromiEvent<TransactionReceipt | TransactionRevertInstructionError> got: PromiEvent<{ status: boolean; transactionHash: string; transactionIndex: number; blockHash: string; blockNumber: number; from: string; to: string; contractAddress?: string | undefined; cumulativeGasUsed: number; gasUsed: number; effectiveGasPrice: number; logs: Log[]; logsBloom: string; events?: { [eventName: string]: EventLog; } | undefined; } | TransactionRevertInstructionError | null> ERROR: 52:1 expect TypeScript@local expected type to be: PromiEvent<TransactionReceipt | TransactionRevertInstructionError> got: PromiEvent<{ status: boolean; transactionHash: string; transactionIndex: number; blockHash: string; blockNumber: number; from: string; to: string; contractAddress?: string | undefined; cumulativeGasUsed: number; gasUsed: number; effectiveGasPrice: number; logs: Log[]; logsBloom: string; events?: { [eventName: string]: EventLog; } | undefined; } | TransactionRevertInstructionError | null> ERROR: 55:1 expect TypeScript@local expected type to be: PromiEvent<TransactionReceipt | TransactionRevertInstructionError> ......

wbt · 2022-03-21T17:18:49Z

@jdevcs Did you see the comment above explaining this?
The tests are pulling expected types being compared to from what is published on NPM, possibly due to its attempt at linearizing a circular dependency, instead of pulling the type definitions from the current package. The actual and expected types are consistent within the package, but the test infrastructure's resolution of a circular dependency doesn't allow that to be checked. (Similarly, if an internal inconsistency were added, the tests could still pass in CI but fail on publish for the same reason.) Reviewing this one will require actually looking at the proposed code change and rationale and reviewing the proposed change directly.

github-actions · 2022-05-21T00:22:24Z