fix: Populate null values in transactions. #2467

ebadiere · 2024-05-08T23:27:19Z

Populates null values in transactions.

Related issue(s):

codecov-commenter · 2024-05-08T23:33:49Z

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 77.79%. Comparing base (07cb2d7) to head (62a04d7).
Report is 2 commits behind head on main.

❗ Current head 62a04d7 differs from pull request most recent head 374e8fd. Consider uploading reports for the commit 374e8fd to get more accurate results

Files	Patch %	Lines
packages/relay/src/lib/eth.ts	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2467      +/-   ##
==========================================
+ Coverage   77.29%   77.79%   +0.50%     
==========================================
  Files          40       27      -13     
  Lines        3228     2585     -643     
  Branches      663      546     -117     
==========================================
- Hits         2495     2011     -484     
+ Misses        524      409     -115     
+ Partials      209      165      -44

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-05-08T23:43:57Z

Acceptance Tests

  20 files 263 suites 24m 53s ⏱️
586 tests 579 ✔️ 3 💤 4 ❌
904 runs 889 ✔️ 7 💤 8 ❌

Results for commit d3f4f64.

♻️ This comment has been updated with latest results.

github-actions · 2024-05-09T13:54:03Z

Tests

    2 files 147 suites 13s ⏱️
821 tests 820 ✔️ 1 💤 0 ❌
833 runs 832 ✔️ 1 💤 0 ❌

Results for commit d3f4f64.

♻️ This comment has been updated with latest results.

Signed-off-by: ebadiere <[email protected]> fix: Added chainId handling and missed file. Signed-off-by: ebadiere <[email protected]> fix: Added ganache encode hex fix. Signed-off-by: ebadiere <[email protected]> fix: Include the ethereumjs-utils in the relay build Signed-off-by: ebadiere <[email protected]> fix: Include package-lock.json. Signed-off-by: ebadiere <[email protected]>

sonarqubecloud · 2024-05-10T14:15:39Z

Quality Gate failed

Failed conditions
3.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

quiet-node

Nice work. Some questions and suggestions

quiet-node · 2024-05-10T16:42:39Z

packages/relay/src/formatters.ts

+  return addHexPrefix(val);
+};
+
+// ganache solution


nit: maybe leave a link to the reference

quiet-node · 2024-05-10T16:46:10Z

packages/relay/src/formatters.ts

@@ -245,26 +280,73 @@ const isValidEthereumAddress = (address: string): boolean => {
  return new RegExp(constants.BASE_HEX_REGEX + '{40}$').test(address);
 };

+// ganache solution


nit: maybe leave a link to the reference

quiet-node · 2024-05-10T16:49:28Z

packages/relay/src/formatters.ts

+  }
+
+  if (typeof val === 'object') {
+    val = val.toString('hex');


still confused by this. How can an object be casted into a string using .toString()? Is val expected to be a special object that has a .toString() method?

there should be a unit test to cover this case in any case, but agree with @quiet-node point

quiet-node · 2024-05-10T16:53:45Z

packages/relay/src/lib/eth.ts

@@ -1888,6 +1889,8 @@ export class EthImpl implements Eth {
    const fromAddress = await this.resolveEvmAddress(contractResult.from, requestIdPrefix, [constants.TYPE_ACCOUNT]);
    const toAddress = await this.resolveEvmAddress(contractResult.to, requestIdPrefix);

+    contractResult.chain_id = contractResult.chain_id == null ? this.chain : contractResult.chain_id;


Just a suggestion

Suggested change

contractResult.chain_id = contractResult.chain_id == null ? this.chain : contractResult.chain_id;

contractResult.chain_id = contractResult.chain_id || this.chain

quiet-node · 2024-05-10T17:00:59Z

packages/relay/src/formatters.ts

+  }
+
+  // Throw error if input type is unexpected
+  throw new Error('Invalid input type. Expected number, string, or null.');


I think this is not necessary. The value argument itself only accept number | string | null, and your if statements already cover all those three scenarios, so this error will never be reached.

quiet-node · 2024-05-10T17:01:53Z

packages/relay/src/formatters.ts

+
+const convertToHex = (value: number | string | null): string => {
+  // Check for null value
+  if (value === null || value === undefined) {


since value only accepts number|string|null, value === undefined would not make sense.

good point @quiet-node,

anyways, the simples way to eval for both null, undefined and empty string would be:

if (!value) {...

Removed value === undefined

quiet-node · 2024-05-10T17:03:08Z

packages/relay/src/lib/eth.ts

@@ -2143,6 +2146,7 @@ export class EthImpl implements Eth {
        constants.TYPE_ACCOUNT,
      ]);
      contractResult.to = await this.resolveEvmAddress(contractResult.to, requestIdPrefix);
+      contractResult.chain_id = contractResult.chain_id == null ? this.chain : contractResult.chain_id;


Just a suggestion

Suggested change

contractResult.chain_id = contractResult.chain_id == null ? this.chain : contractResult.chain_id;

contractResult.chain_id = contractResult.chain_id || this.chain

AlfredoG87

LG, agreed with a few nits from @quiet-node otherwise looking good.

AlfredoG87 · 2024-05-11T01:50:10Z

packages/relay/src/formatters.ts

+
+const convertToHex = (value: number | string | null): string => {
+  // Check for null value
+  if (value === null || value === undefined) {


good point @quiet-node,

anyways, the simples way to eval for both null, undefined and empty string would be:

if (!value) {...

AlfredoG87 · 2024-05-11T01:53:42Z

packages/relay/src/formatters.ts

+  }
+
+  if (typeof val === 'object') {
+    val = val.toString('hex');


there should be a unit test to cover this case in any case, but agree with @quiet-node point

ebadiere · 2024-05-15T03:04:02Z

Replaced by #2495

ebadiere added the bug Something isn't working label May 8, 2024

ebadiere added this to the 0.48.0 milestone May 8, 2024

ebadiere self-assigned this May 8, 2024

ebadiere requested review from AlfredoG87, lukelee-sl and a team as code owners May 8, 2024 23:27

ebadiere force-pushed the 2451-maxPriorityFeePerGas-missing-getBlockByNumber branch from bd765e0 to d3f4f64 Compare May 10, 2024 14:14

quiet-node reviewed May 10, 2024

View reviewed changes

AlfredoG87 reviewed May 11, 2024

View reviewed changes

ebadiere closed this May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Populate null values in transactions. #2467

fix: Populate null values in transactions. #2467

ebadiere commented May 8, 2024

codecov-commenter commented May 8, 2024

github-actions bot commented May 8, 2024 •

edited

Loading

github-actions bot commented May 9, 2024 •

edited

Loading

sonarqubecloud bot commented May 10, 2024

quiet-node left a comment

quiet-node May 10, 2024

ebadiere May 13, 2024

quiet-node May 10, 2024

ebadiere May 13, 2024

quiet-node May 10, 2024 •

edited

Loading

AlfredoG87 May 11, 2024

quiet-node May 10, 2024

ebadiere May 13, 2024

quiet-node May 10, 2024

ebadiere May 13, 2024

quiet-node May 10, 2024

AlfredoG87 May 11, 2024

ebadiere May 13, 2024

quiet-node May 10, 2024

ebadiere May 13, 2024

AlfredoG87 left a comment

AlfredoG87 May 11, 2024

AlfredoG87 May 11, 2024

ebadiere commented May 15, 2024

	contractResult.chain_id = contractResult.chain_id == null ? this.chain : contractResult.chain_id;
	contractResult.chain_id = contractResult.chain_id \|\| this.chain

fix: Populate null values in transactions. #2467

fix: Populate null values in transactions. #2467

Conversation

ebadiere commented May 8, 2024

codecov-commenter commented May 8, 2024

Codecov Report

github-actions bot commented May 8, 2024 • edited Loading

Acceptance Tests

github-actions bot commented May 9, 2024 • edited Loading

Tests

sonarqubecloud bot commented May 10, 2024

Quality Gate failed

quiet-node left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quiet-node May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlfredoG87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebadiere commented May 15, 2024

github-actions bot commented May 8, 2024 •

edited

Loading

github-actions bot commented May 9, 2024 •

edited

Loading

quiet-node May 10, 2024 •

edited

Loading