Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nullable check to estimateGas validation #849

Merged
merged 7 commits into from
Jan 27, 2023

Conversation

Nana-EC
Copy link
Collaborator

@Nana-EC Nana-EC commented Jan 26, 2023

Signed-off-by: Nana Essilfie-Conduah [email protected]

Description:
MM seems to send a null data param in the transaction object.
We should not be validating in that case.

  • Expand trace logs to eth.estimateGas() to highlight param
  • Add nullable concept to validation object params and set data in transaction object to nullable
  • Expand predefined.INVALID_PARAMETER logs to make failures easier to troubleshoot
  • Updated and expanded tests to confirm isValidAndNonNullableParam logic
  • Updated RPC tests impacted by addition of value to the log

Related issue(s):

Fixes #836

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@Nana-EC Nana-EC self-assigned this Jan 26, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2023

Codecov Report

Base: 73.70% // Head: 73.73% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (c8fcb69) compared to base (57f73c0).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@               Coverage Diff                @@
##           release/0.16     #849      +/-   ##
================================================
+ Coverage         73.70%   73.73%   +0.02%     
================================================
  Files                29       29              
  Lines              1780     1782       +2     
  Branches            323      324       +1     
================================================
+ Hits               1312     1314       +2     
  Misses              374      374              
  Partials             94       94              
Impacted Files Coverage Δ
packages/server/src/validator/objectTypes.ts 100.00% <ø> (ø)
packages/relay/src/lib/eth.ts 82.01% <100.00%> (ø)
packages/server/src/validator/utils.ts 88.09% <100.00%> (+0.59%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Nana-EC Nana-EC requested a review from lukelee-sl January 26, 2023 23:36
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
lukelee-sl
lukelee-sl previously approved these changes Jan 27, 2023
Copy link
Member

@lukelee-sl lukelee-sl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but some failing tests.

@lukelee-sl lukelee-sl self-requested a review January 27, 2023 01:41
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Nana-EC and others added 2 commits January 26, 2023 20:26
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@georgi-l95 georgi-l95 marked this pull request as ready for review January 27, 2023 07:18
Copy link
Collaborator

@georgi-l95 georgi-l95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

@Nana-EC Nana-EC added bug Something isn't working P1 dev tools Features enabling dev tool integration labels Jan 27, 2023
@Nana-EC Nana-EC added this to the 0.16.0 milestone Jan 27, 2023
@Nana-EC Nana-EC merged commit 22a61a5 into release/0.16 Jan 27, 2023
@Nana-EC Nana-EC deleted the 836-0-16-estimateGas-valdation branch January 27, 2023 08:16
Nana-EC added a commit that referenced this pull request Jan 27, 2023
MM seems to send a null data param in the transaction object.
We should not be validating in that case.

- Expand trace logs to `eth.estimateGas()` to highlight param
- Add nullable concept to validation object params and set data in transaction object to nullable
- Expand `predefined.INVALID_PARAMETER` logs to make failures easier to troubleshoot
- Updated and expanded tests to confirm `isValidAndNonNullableParam` logic
- Updated RPC tests impacted by addition of value to the log

Signed-off-by: georgi-l95 <[email protected]>

Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Co-authored-by: georgi-l95 <[email protected]>
Nana-EC added a commit that referenced this pull request Jan 27, 2023
MM seems to send a null data param in the transaction object.
We should not be validating in that case.

- Expand trace logs to `eth.estimateGas()` to highlight param
- Add nullable concept to validation object params and set data in transaction object to nullable
- Expand `predefined.INVALID_PARAMETER` logs to make failures easier to troubleshoot
- Updated and expanded tests to confirm `isValidAndNonNullableParam` logic
- Updated RPC tests impacted by addition of value to the log

Signed-off-by: georgi-l95 <[email protected]>

Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Co-authored-by: georgi-l95 <[email protected]>
Nana-EC added a commit that referenced this pull request Jan 30, 2023
MM seems to send a null data param in the transaction object.
We should not be validating in that case.

- Expand trace logs to `eth.estimateGas()` to highlight param
- Add nullable concept to validation object params and set data in transaction object to nullable
- Expand `predefined.INVALID_PARAMETER` logs to make failures easier to troubleshoot
- Updated and expanded tests to confirm `isValidAndNonNullableParam` logic
- Updated RPC tests impacted by addition of value to the log

Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Co-authored-by: georgi-l95 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dev tools Features enabling dev tool integration P1
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants