Improved schema validation for eth_getBalance #598

cygnusv · 2024-10-18T15:33:40Z

Type of PR:

Bugfix
Feature
Documentation
Other

Required reviews:

1
2
3

What this does:

High-level idea of the changes introduced in this PR. List relevant API
changes (if any), as well as related PRs and issues.

Issues fixed/closed:

Fixes #...

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network. E.g.,
if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

What should reviewers focus on? Is there a particular commit/function/section
of your PR that requires more attention from reviewers?

* Allow any type of context variable for the eth address parameter * The non-empty array is not an acceptable schema for parameters. What we want is a one-element tuple

netlify · 2024-10-18T15:33:57Z

✅ Deploy Preview for taco-demo canceled.

Name	Link
🔨 Latest commit	`0d750e4`
🔍 Latest deploy log	https://app.netlify.com/sites/taco-demo/deploys/67127fd7eaba4200083e5056

netlify · 2024-10-18T15:33:58Z

✅ Deploy Preview for taco-nft-demo canceled.

Name	Link
🔨 Latest commit	`0d750e4`
🔍 Latest deploy log	https://app.netlify.com/sites/taco-nft-demo/deploys/67127fd7cb6ea10008709f5d

See https://ethereum.github.io/execution-apis/api-documentation/

codecov-commenter · 2024-10-18T15:48:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (epic-v0.6.x@fcdb1da). Learn more about missing BASE report.

Additional details and impacted files

@@              Coverage Diff               @@
##             epic-v0.6.x     #598   +/-   ##
==============================================
  Coverage               ?   89.19%           
==============================================
  Files                  ?       73           
  Lines                  ?     6498           
  Branches               ?      345           
==============================================
  Hits                   ?     5796           
  Misses                 ?      664           
  Partials               ?       38

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

packages/shared/src/schemas.ts

packages/taco/test/conditions/base/rpc.test.ts

derekpierre · 2024-10-24T17:00:47Z

packages/taco/src/conditions/schemas/context.ts

@@ -6,7 +6,8 @@ import { plainStringSchema } from './common';

 export const contextParamSchema = z.string().regex(CONTEXT_PARAM_REGEXP);

-const paramSchema = z.union([plainStringSchema, z.boolean(), z.number()]);
+// TODO: This is too broad, but it's a start
+const paramSchema = z.union([plainStringSchema, z.boolean(), z.number().int().nonnegative()]);


Is there ever a case for negative numbers?

You're right, it is indeed acceptable (int256 type and friends). Fixed in b3902d4

I was thinking about this some more. Perhaps we need to separate this schema into two parts.

What is allowed for blockchain RPC / smart contract calls

What is generally allowed

The original goal of the paraSchema on L9 was 2), but we are now limiting it to 1).

Solidity doesn't have floating point numbers, so limiting it to z.number().int() is fine. However, a JsonApiCondition can require floating point numbers. For example, the paramOrContextParamSchema is used for the value used for the returnValueTest () - and for a JSON API, checking a floating point number is perfectly fine, but now it won't be; typified by the fact that I saw you had to modify the floating point test.

^ @cygnusv wdyt?

yep, I was actually reverting my changes as we speak. Let's just put a pin on this so we can unblock the PR.

Opened #600

manumonti

LGTM! 👌

Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

derekpierre

🎸

refactor: improvements to eth_getBalance schema

3ecfeed

* Allow any type of context variable for the eth address parameter * The non-empty array is not an acceptable schema for parameters. What we want is a one-element tuple

cygnusv changed the base branch from main to epic-v0.6.x October 18, 2024 15:33

cygnusv added 4 commits October 18, 2024 17:44

refactor: Don't use eth address regex, use existing schema instead

978f56b

test: improve eth address schema unit tests

f6084df

feat: new BlockIdentifierSchema

942258b

See https://ethereum.github.io/execution-apis/api-documentation/

feat: Add block identifier schema validation to eth_getBalance

677e7e7

cygnusv force-pushed the blocknumber branch from 0d750e4 to 677e7e7 Compare October 18, 2024 15:44

derekpierre reviewed Oct 18, 2024

View reviewed changes

packages/shared/src/schemas.ts Outdated Show resolved Hide resolved

packages/taco/test/conditions/base/rpc.test.ts Show resolved Hide resolved

cygnusv marked this pull request as ready for review October 24, 2024 16:50

cygnusv requested review from derekpierre, KPrasch and manumonti October 24, 2024 16:50

derekpierre reviewed Oct 24, 2024

View reviewed changes

manumonti approved these changes Oct 25, 2024

View reviewed changes

derekpierre mentioned this pull request Oct 28, 2024

feat: allow authorization token for JsonApiCondition to support endpoints requiring OAuth, JWT authorization #599

Merged

7 tasks

derekpierre assigned cygnusv Oct 29, 2024

cygnusv and others added 2 commits October 29, 2024 16:54

fix: contextParamSchema is already exported by context.ts

fa81b97

feat: Add error message to BlockHash schema validation

f971fea

Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

cygnusv force-pushed the blocknumber branch from b3902d4 to f971fea Compare October 29, 2024 15:55

cygnusv mentioned this pull request Oct 29, 2024

Consider more restrictive schemas for EVM base types #600

Open

derekpierre approved these changes Oct 29, 2024

View reviewed changes

cygnusv merged commit 3b67142 into nucypher:epic-v0.6.x Oct 29, 2024
4 checks passed

derekpierre mentioned this pull request Nov 8, 2024

[EPIC] v0.6.x #592

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved schema validation for eth_getBalance #598

Improved schema validation for eth_getBalance #598

cygnusv commented Oct 18, 2024 •

edited by derekpierre

Loading

netlify bot commented Oct 18, 2024 •

edited

Loading

netlify bot commented Oct 18, 2024 •

edited

Loading

codecov-commenter commented Oct 18, 2024 •

edited

Loading

derekpierre Oct 24, 2024

cygnusv Oct 25, 2024

derekpierre Oct 25, 2024 •

edited

Loading

derekpierre Oct 29, 2024

cygnusv Oct 29, 2024

cygnusv Oct 29, 2024

manumonti left a comment

derekpierre left a comment

Improved schema validation for eth_getBalance #598

Improved schema validation for eth_getBalance #598

Conversation

cygnusv commented Oct 18, 2024 • edited by derekpierre Loading

netlify bot commented Oct 18, 2024 • edited Loading

✅ Deploy Preview for taco-demo canceled.

netlify bot commented Oct 18, 2024 • edited Loading

✅ Deploy Preview for taco-nft-demo canceled.

codecov-commenter commented Oct 18, 2024 • edited Loading

Codecov Report

derekpierre Oct 24, 2024

Choose a reason for hiding this comment

cygnusv Oct 25, 2024

Choose a reason for hiding this comment

derekpierre Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

derekpierre Oct 29, 2024

Choose a reason for hiding this comment

cygnusv Oct 29, 2024

Choose a reason for hiding this comment

cygnusv Oct 29, 2024

Choose a reason for hiding this comment

manumonti left a comment

Choose a reason for hiding this comment

derekpierre left a comment

Choose a reason for hiding this comment

cygnusv commented Oct 18, 2024 •

edited by derekpierre

Loading

netlify bot commented Oct 18, 2024 •

edited

Loading

netlify bot commented Oct 18, 2024 •

edited

Loading

codecov-commenter commented Oct 18, 2024 •

edited

Loading

derekpierre Oct 25, 2024 •

edited

Loading