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

Improved schema validation for eth_getBalance #598

Draft
wants to merge 5 commits into
base: epic-v0.6.x
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions packages/shared/src/schemas.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { ethers } from 'ethers';
import { z } from 'zod';

export const ETH_ADDRESS_REGEXP = new RegExp('^0x[a-fA-F0-9]{40}$');

const isAddress = (address: string) => {
try {
return ethers.utils.getAddress(address);
Expand All @@ -13,5 +11,11 @@ const isAddress = (address: string) => {

export const EthAddressSchema = z
.string()
.regex(ETH_ADDRESS_REGEXP)
.refine(isAddress, { message: 'Invalid Ethereum address' });

const BLOCK_HASH_REGEXP = new RegExp('^0x[a-fA-F0-9]{64}$');
const BlockNumber = z.number().int().nonnegative();
const BlockHash = z.string().regex(BLOCK_HASH_REGEXP);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const BlockHash = z.string().regex(BLOCK_HASH_REGEXP);
const BlockHash = z.string().regex(BLOCK_HASH_REGEXP, 'Invalid block hash');

const BlockTag = z.enum(['earliest', 'finalized', 'safe', 'latest', 'pending']);

export const BlockIdentifierSchema = z.union([BlockNumber, BlockHash, BlockTag]);
58 changes: 56 additions & 2 deletions packages/shared/test/schemas.test.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,69 @@
import { describe, expect, it } from 'vitest';

import { EthAddressSchema } from '../src';
import { EthAddressSchema, BlockIdentifierSchema } from '../src';

describe('ethereum address schema', () => {
it('should accept valid ethereum address', () => {
const validAddress = '0x1234567890123456789012345678901234567890';
EthAddressSchema.parse(validAddress);
});

it('should reject invalid ethereum address', () => {
it('should accept unchecksummed ethereum address', () => {
const validAddress = '0x0123456789abcdefedcb0123456789abcdefedcb';
EthAddressSchema.parse(validAddress);
});

it('should accept checksummed ethereum address', () => {
const validAddress = '0x0123456789aBcDeFEdCb0123456789abcdEfeDcb';
EthAddressSchema.parse(validAddress);
});

it('should reject invalid ethereum address (shorter)', () => {
const invalidAddress = '0x123456789012345678901234567890123456789';
expect(() => EthAddressSchema.parse(invalidAddress)).toThrow();
});

it('should reject invalid ethereum address (longer)', () => {
const invalidAddress = '0x12345678901234567890123456789012345678901';
expect(() => EthAddressSchema.parse(invalidAddress)).toThrow();
});
});

describe('block identifier address schema', () => {
it('should accept valid block numbers (ints >= 0)', () => {
BlockIdentifierSchema.parse(0);
BlockIdentifierSchema.parse(1);
BlockIdentifierSchema.parse(1234);
BlockIdentifierSchema.parse(9007199254740991); // Max safe integer
});

it('should accept valid block tags', () => {
BlockIdentifierSchema.parse('earliest');
BlockIdentifierSchema.parse('finalized');
BlockIdentifierSchema.parse('safe');
BlockIdentifierSchema.parse('latest');
BlockIdentifierSchema.parse('pending');
});

it('should accept valid block hashes', () => {
const validBlockHash = '0x1234567890123456789012345678901234567890123456789012345678901234';
BlockIdentifierSchema.parse(validBlockHash);
});

it('should reject invalid block numbers (e.g., negative ints)', () => {
expect(() => BlockIdentifierSchema.parse(-42)).toThrow();
});

it('should reject invalid block numbers (e.g., float)', () => {
expect(() => BlockIdentifierSchema.parse(34.56)).toThrow();
});

it('should reject invalid block identifiers', () => {
expect(() => BlockIdentifierSchema.parse('foo')).toThrow();
});

it('should reject invalid block hashes', () => {
const invalidBlockHash = '0x1234';
expect(() => BlockIdentifierSchema.parse(invalidBlockHash)).toThrow();
});
});
6 changes: 0 additions & 6 deletions packages/taco/src/conditions/schemas/common.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { EthAddressSchema } from '@nucypher/shared';
import {
USER_ADDRESS_PARAM_DEFAULT,
USER_ADDRESS_PARAM_EXTERNAL_EIP4361,
Expand All @@ -25,11 +24,6 @@ export const UserAddressSchema = z.enum([
USER_ADDRESS_PARAM_EXTERNAL_EIP4361,
]);

export const EthAddressOrUserAddressSchema = z.union([
EthAddressSchema,
UserAddressSchema,
]);

export const baseConditionSchema = z.object({
conditionType: z.string(),
});
Expand Down
4 changes: 2 additions & 2 deletions packages/taco/src/conditions/schemas/contract.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ETH_ADDRESS_REGEXP } from '@nucypher/shared';
import { EthAddressSchema } from '@nucypher/shared';
import { ethers } from 'ethers';
import { z } from 'zod';

Expand Down Expand Up @@ -76,7 +76,7 @@ export const contractConditionSchema = rpcConditionSchema
conditionType: z
.literal(ContractConditionType)
.default(ContractConditionType),
contractAddress: z.string().regex(ETH_ADDRESS_REGEXP).length(42),
contractAddress: EthAddressSchema,
standardContractType: z.enum(['ERC20', 'ERC721']).optional(),
method: z.string(),
functionAbi: functionAbiSchema.optional(),
Expand Down
22 changes: 17 additions & 5 deletions packages/taco/src/conditions/schemas/rpc.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,36 @@
import { BlockIdentifierSchema, EthAddressSchema } from '@nucypher/shared';
import { z } from 'zod';

import { SUPPORTED_CHAIN_IDS } from '../const';

import createUnionSchema, {
baseConditionSchema,
EthAddressOrUserAddressSchema,
UserAddressSchema,
} from './common';
import { paramOrContextParamSchema } from './context';
import { contextParamSchema } from './context';
import { returnValueTestSchema } from './return-value-test';

export const RpcConditionType = 'rpc';

const EthAddressOrContextVariableSchema = z.union([
EthAddressSchema,
UserAddressSchema,
contextParamSchema
]);
const BlockOrContextParamSchema = z.union([BlockIdentifierSchema, contextParamSchema])

// eth_getBalance schema specification
// - Ethereum spec: https://ethereum.github.io/execution-apis/api-documentation/
// - web3py: https://web3py.readthedocs.io/en/stable/web3.eth.html#web3.eth.Eth.get_balance
export const rpcConditionSchema = baseConditionSchema.extend({
conditionType: z.literal(RpcConditionType).default(RpcConditionType),
chain: createUnionSchema(SUPPORTED_CHAIN_IDS),
method: z.enum(['eth_getBalance']),
parameters: z.union([
z.array(EthAddressOrUserAddressSchema).nonempty(),
// Using tuple here because ordering matters
z.tuple([EthAddressOrUserAddressSchema, paramOrContextParamSchema]),
// Spec requires 2 parameters: an address and a block identifier
z.tuple([EthAddressOrContextVariableSchema, BlockOrContextParamSchema]),
// Block identifier can be omitted, since web3py (which runs on TACo exec layer) defaults to 'latest',
z.tuple([EthAddressOrContextVariableSchema]),
]),
returnValueTest: returnValueTestSchema, // Update to allow multiple return values after expanding supported methods
});
Expand Down
2 changes: 1 addition & 1 deletion packages/taco/test/conditions/base/condition.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('validation', () => {
expect(result.data).toBeUndefined();
expect(result.error?.format()).toMatchObject({
contractAddress: {
_errors: ['Invalid', 'String must contain exactly 42 character(s)'],
_errors: ['Invalid Ethereum address'],
},
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/taco/test/conditions/base/contract.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ describe('supports custom function abi', async () => {
it.each([
{
contractAddress: '0x123',
error: ['Invalid', 'String must contain exactly 42 character(s)'],
error: ['Invalid Ethereum address'],
},
{ contractAddress: undefined, error: ['Required'] },
])('rejects invalid contract address', async ({ contractAddress, error }) => {
Expand Down
52 changes: 43 additions & 9 deletions packages/taco/test/conditions/base/rpc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,55 @@ describe('validation', () => {
});
});

it('accepts a single UserAddress as address', () => {
const result = RpcCondition.validate(rpcConditionSchema, {
...testRpcConditionObj,
parameters: [":userAddress"],
});

expect(result.error).toBeUndefined();
expect(result.data).toEqual({
...testRpcConditionObj,
parameters: [":userAddress"],
});
});

it('accepts a single context variable as address', () => {
const result = RpcCondition.validate(rpcConditionSchema, {
...testRpcConditionObj,
parameters: [":testContextVar"],
});

expect(result.error).toBeUndefined();
expect(result.data).toEqual({
...testRpcConditionObj,
parameters: [":testContextVar"],
});
});

it('accepts a single address and a block number', () => {
const result = RpcCondition.validate(rpcConditionSchema, {
...testRpcConditionObj,
parameters: [TEST_CONTRACT_ADDR, 'latest'],
parameters: [TEST_CONTRACT_ADDR, 2345],
});

expect(result.error).toBeUndefined();
expect(result.data).toEqual({
...testRpcConditionObj,
parameters: [TEST_CONTRACT_ADDR, 2345],
});
});

it('accepts context params for address and block number', () => {
const result = RpcCondition.validate(rpcConditionSchema, {
...testRpcConditionObj,
parameters: [":testAddress", ":testBlockNumber"],
});

expect(result.error).toBeUndefined();
expect(result.data).toEqual({
...testRpcConditionObj,
parameters: [TEST_CONTRACT_ADDR, 'latest'],
parameters: [":testAddress", ":testBlockNumber"],
});
});

Expand All @@ -88,12 +127,7 @@ describe('validation', () => {
expect(result.data).toBeUndefined();
expect(result.error?.format()).toMatchObject({
parameters: {
'1': {
_errors: ['Invalid', 'Invalid Ethereum address'],
},
'2': {
_errors: ['Invalid', 'Invalid Ethereum address'],
},
_errors: ['Array must contain at most 2 element(s)'],
},
});
});
Expand All @@ -109,7 +143,7 @@ describe('validation', () => {
expect(result.data).toBeUndefined();
expect(result.error?.format()).toMatchObject({
parameters: {
_errors: ['Array must contain at least 1 element(s)'],
_errors: ['Array must contain at least 2 element(s)', 'Array must contain at least 1 element(s)'],
Copy link
Member

@derekpierre derekpierre Oct 18, 2024

Choose a reason for hiding this comment

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

I did some digging around to see if there is a way to work around this - it's a little conflicting to have both i.e. at least 2 element(s) and at least 1 element(s) - but I couldn't find anything. Seems to be by design - colinhacks/zod#311 (comment).

},
});
});
Expand Down