monorepo: remove isTruthy and isFalsy usage #2233
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
gabrocheleau
changed the title
| @@ -33,7 +31,7 @@ export class LevelDB implements DB { | |||
| try { | |||
| value = await this._leveldb.get(key, ENCODING_OPTS) | |||
| } catch (error: any) { | |||
| if (isTruthy(error.notFound)) { | |||
| if (error.notFound !== undefined) { | |||
| // not found, returning null | |||
There was a problem hiding this comment.
Does providing no return value mean we return null or undefined?
There was a problem hiding this comment.
No return is treated as undefined by JS
There was a problem hiding this comment.
@gabrocheleau https://github.com/Level/abstract-level/blob/915ad1317694d0ce8c580b5ab85d81e1e78a3137/abstract-level.js#L309 maybe do an explicit true check here because it should only be true if leveldb really added it.
There was a problem hiding this comment.
Then shouldn't we be explicitly returning null here if we want that? If it were me, I'd remove null from the monorepo entirely but we're not there yet.
There was a problem hiding this comment.
I would probably change that whole block to get the null return and be more correct on the check. With the below code we would just fall through to return value in every case and its default value is null already.
async get(key: Buffer): Promise<Buffer | null> {
let value = null
try {
value = await this._leveldb.get(key, ENCODING_OPTS)
} catch (error: any) {
// https://github.com/Level/abstract-level/blob/915ad1317694d0ce8c580b5ab85d81e1e78a3137/abstract-level.js#L309
// This should be `true` if the error came from LevelDB
// so we can check for `NOT true` to identify any non-404 errors
if (error.notFound !== true) {
throw error
}
}
return value as Buffer
}There was a problem hiding this comment.
I've simplified that catch/try logic and am now explicitly checking for true. I've also typecasted to Buffer | null instead of just Buffer, which matches the return type of the function. In this particular case value is initialized as null, and get() throws when not found, so value would never be undefined.
One side note though is that the typecasting is necessary because get() returns string | Buffer. Are we confident that get won't ever return a string in this particular instance? If not, shouldn't we convert to buffer instead of typecasting?
There was a problem hiding this comment.
LevelDB generally returns what you insert and the encoding is set to buffer/binary so it'll be forced to always return Buffer. Unless there is a bug in LevelDB you'll always have Buffer after calling get.
| @@ -271,7 +274,9 @@ export class Miner { | |||
| const txs = await this.service.txPool.txsByPriceAndNonce(baseFeePerGas) | |||
| this.config.logger.info( | |||
| `Miner: Assembling block from ${txs.length} eligible txs ${ | |||
| isTruthy(baseFeePerGas) ? `(baseFee: ${baseFeePerGas})` : '' | |||
| typeof baseFeePerGas === 'bigint' && baseFeePerGas !== BigInt(0) | |||
There was a problem hiding this comment.
Maybe this is a silly question but we wouldn't the check to be baseFeePerGas > BigInt(0) right? I'm assuming there's reality in which it could be less in the miner's code?
There was a problem hiding this comment.
Not sure I understand the question, you mean a negative baseFeePerGas?
There was a problem hiding this comment.
Yes, disregard. I wasn't thinking clearly when I made this comment,
| @@ -30,11 +30,6 @@ const debugGas = createDebugLogger('vm:tx:gas') | |||
| * @ignore | |||
| */ | |||
| export async function runTx(this: VM, opts: RunTxOpts): Promise<RunTxResult> { | |||
| // tx is required | |||
There was a problem hiding this comment.
Is there some reason we're removing this check?
There was a problem hiding this comment.
Yep, see comment from Jochem here: #2233 (comment)
basically tx is a non-optional option so opts.tx should never be undefined here
|
@gabrocheleau thanks for this great work, totally fantastic! 🎉 Can we please have this initial PR split up after this first round of reviews and be submitted package-by-package as being done for the ESLint changes in #2147? This is otherwise too many code changes on too many files in one PR and at the same time highly sensitive on every code change, which is somewhat of a toxic combination. This otherwise just naturally leads to too superficial reviews. |
Agreed, done! |
|
Closing in favor of the package-specific PRs |
🙏 Thanks a lot for the understanding! |
This PR removes the isTruthy/isFalsy usage from the following packages
It also makes minor refactors/improvements when the updated types allow for it.