Skip to content

v1: Signature audit pt 1 #2360

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

Open
wants to merge 12 commits into
base: onchainkit-v1
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions packages/onchainkit/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"graphql-request": "^6.1.0",
"qrcode": "^1.5.4",
"tailwind-merge": "^2.3.0",
"usehooks-ts": "^3.1.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation of useIsMounted in src/internal/hooks/useIsMounted doesn't let you check if the component is mounted at the end of the component lifecycle.

Rather than update the hook and its usage internally, I figured bringing in this library would be better since it also covers a lot of the hooks we have in our codebase, and it's been extremely battle tested.

"viem": "^2.27.2",
"wagmi": "^2.14.11"
},
Expand Down
10 changes: 8 additions & 2 deletions packages/onchainkit/src/internal/hooks/useLifecycleStatus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@ import { useCallback, useState } from 'react';

export function useLifecycleStatus<T extends AbstractLifecycleStatus>(
initialState: T,
options?: {
onStatus?: (status: T) => void;
},
): UseLifecycleStatusReturn<T> {
const [lifecycleStatus, setLifecycleStatus] = useState<T>(initialState); // Component lifecycle
const { onStatus } = options ?? {};

// Update lifecycle status, statusData will be persisted for the full lifecycle
const updateLifecycleStatus = useCallback(
Expand All @@ -22,16 +26,18 @@ export function useLifecycleStatus<T extends AbstractLifecycleStatus>(
prevStatus.statusData,
)
: prevStatus.statusData;
return {
const nextStatus = {
statusName: newStatus.statusName,
statusData: {
...persistedStatusData,
...newStatus.statusData,
},
} as T;
onStatus?.(nextStatus);
return nextStatus;
});
},
[],
[onStatus],
);

return [lifecycleStatus, updateLifecycleStatus];
Expand Down
31 changes: 30 additions & 1 deletion packages/onchainkit/src/internal/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { APIError } from '@/api/types';
import { ReactNode } from 'react';

type ErrorStatus = {
statusName: 'error';
Expand All @@ -10,7 +11,7 @@ type GenericStatus<T> = {
statusData: T;
};

/* eslint-disable @typescript-eslint/no-explicit-any */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type AbstractLifecycleStatus = ErrorStatus | GenericStatus<any>;

export type UseLifecycleStatusReturn<T extends AbstractLifecycleStatus> = [
Expand Down Expand Up @@ -66,3 +67,31 @@ export type LifecycleStatusUpdate<T extends AbstractLifecycleStatus> =
*/
export type MakeRequired<T, K extends keyof T> = Omit<T, K> &
Required<Pick<T, K>>;

/**
* Utility for typing component props that support the render prop pattern.
* Returns a type where the render prop and the props it may clash with
* are separated as a discriminated union.
*
* It defaults the clashing props to `className` and `children`.
* This can be overridden by passing a second argument to the generic.
*/
export type WithRenderProps<
TProps extends {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
render?: (arg: any) => ReactNode;
},
TExclude extends string = 'className' | 'children',
> = Omit<TProps, TExclude | 'render'> &
(
| ({
render?: TProps['render'];
} & {
[K in TExclude]?: undefined;
})
| ({
render?: never;
} & {
[K in TExclude]?: K extends keyof TProps ? TProps[K] : never;
})
);
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,24 @@ import { render, screen } from '@testing-library/react';
import { encodeAbiParameters } from 'viem';
import { base } from 'viem/chains';
import { type Mock, beforeEach, describe, expect, it, vi } from 'vitest';
import { Signature } from './Signature';
import { Signature } from '../components/Signature';

vi.mock('@/internal/hooks/useTheme', () => ({
useTheme: vi.fn(() => 'default-light'),
}));
vi.mock('@/internal/hooks/useIsMounted');
vi.mock('./SignatureProvider', () => ({
vi.mock('../components/SignatureProvider', () => ({
SignatureProvider: vi.fn(({ children }) => (
<div data-testid="SignatureProvider">{children}</div>
)),
}));
vi.mock('./SignatureStatus', () => ({
vi.mock('../components/SignatureStatus', () => ({
SignatureStatus: vi.fn(() => <div>SignatureStatus</div>),
}));
vi.mock('./SignatureToast', () => ({
vi.mock('../components/SignatureToast', () => ({
SignatureToast: vi.fn(() => <div>SignatureToast</div>),
}));
vi.mock('./SignatureButton', () => ({
vi.mock('../components/SignatureButton', () => ({
SignatureButton: vi.fn(({ label, disabled }) => (
<button type="button" disabled={disabled}>
{label}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@ import {
vi,
} from 'vitest';
import { useAccount, useConnect } from 'wagmi';
import { SignatureButton } from './SignatureButton';
import { useSignatureContext } from './SignatureProvider';
import { SignatureButton } from '../components/SignatureButton';
import { useSignatureContext } from '../components/SignatureProvider';

vi.mock('wagmi', () => ({
useAccount: vi.fn(),
useConnect: vi.fn(),
useConnectors: vi.fn(() => ({ connectors: [{ id: 'mockConnector' }] })),
}));

vi.mock('./SignatureProvider', () => ({
vi.mock('../components/SignatureProvider', () => ({
useSignatureContext: vi.fn(),
}));

Expand Down Expand Up @@ -59,8 +59,8 @@ describe('SignatureButton', () => {
status: 'disconnected',
});

render(<SignatureButton connectLabel="Sign in please" />);
expect(screen.getByText('Sign in please')).toBeInTheDocument();
render(<SignatureButton disconnectedLabel="Disconnected Label" />);
expect(screen.getByText('Disconnected Label')).toBeInTheDocument();
});

it('should render sign button when address is connected', () => {
Expand Down Expand Up @@ -172,4 +172,84 @@ describe('SignatureButton', () => {
render(<SignatureButton />);
expect(screen.getByText('Sign')).toBeInTheDocument();
});

it('should use custom render function when provided', () => {
(useAccount as Mock).mockReturnValue({ address: '0x123' });
const mockContext = {
lifecycleStatus: {
statusName: 'init',
},
handleSign: mockHandleSign,
};
(useSignatureContext as Mock).mockReturnValue(mockContext);

const customRender = vi.fn(({ label, onClick, context }) => {
expect(context).toBe(mockContext);
return (
<button data-testid="custom-button" onClick={onClick}>
Custom: {label}
</button>
);
});

render(<SignatureButton label="Sign Message" render={customRender} />);

expect(customRender).toHaveBeenCalledWith({
label: 'Sign Message',
onClick: mockHandleSign,
context: mockContext,
});
expect(screen.getByTestId('custom-button')).toBeInTheDocument();
expect(screen.getByText('Custom: Sign Message')).toBeInTheDocument();
});

it('should trigger handleSign when custom rendered button is clicked', () => {
(useAccount as Mock).mockReturnValue({ address: '0x123' });
(useSignatureContext as Mock).mockReturnValue({
lifecycleStatus: {
statusName: 'init',
},
handleSign: mockHandleSign,
});

render(
<SignatureButton
label="Sign Message"
render={({ label, onClick }) => (
<button data-testid="custom-button" onClick={onClick}>
{label}
</button>
)}
/>,
);

fireEvent.click(screen.getByTestId('custom-button'));
expect(mockHandleSign).toHaveBeenCalled();
});

it('should pass updated label to render function based on lifecycle status', () => {
(useAccount as Mock).mockReturnValue({ address: '0x123' });
(useSignatureContext as Mock).mockReturnValue({
lifecycleStatus: {
statusName: 'success',
},
handleSign: mockHandleSign,
});

const customRender = vi.fn(({ label }) => (
<div data-testid="rendered-label">{label}</div>
));

render(
<SignatureButton
label="Sign Message"
successLabel="Successfully Signed"
render={customRender}
/>,
);

expect(screen.getByTestId('rendered-label')).toHaveTextContent(
'Successfully Signed',
);
});
});
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { render, screen } from '@testing-library/react';
import { type Mock, describe, expect, it, vi } from 'vitest';
import { MessageType } from '../types';
import { SignatureIcon } from './SignatureIcon';
import { useSignatureContext } from './SignatureProvider';
import { SignatureIcon } from '../components/SignatureIcon';
import { useSignatureContext } from '../components/SignatureProvider';

vi.mock('./SignatureProvider', () => ({
vi.mock('../components/SignatureProvider', () => ({
useSignatureContext: vi.fn(),
}));
vi.mock('../../internal/svg/errorSvg', () => ({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { render, screen } from '@testing-library/react';
import { type Mock, describe, expect, it, vi } from 'vitest';
import { MessageType } from '../types';
import { SignatureLabel } from './SignatureLabel';
import { useSignatureContext } from './SignatureProvider';
import { SignatureLabel } from '../components/SignatureLabel';
import { useSignatureContext } from '../components/SignatureProvider';

vi.mock('./SignatureProvider', () => ({
vi.mock('../components/SignatureProvider', () => ({
useSignatureContext: vi.fn(),
}));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ import {
vi,
} from 'vitest';
import { useSignMessage, useSignTypedData } from 'wagmi';
import { SignatureProvider, useSignatureContext } from './SignatureProvider';
import {
SignatureProvider,
useSignatureContext,
} from '../components/SignatureProvider';

vi.mock('wagmi', () => ({
useSignMessage: vi.fn(),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { render, screen } from '@testing-library/react';
import { type Mock, beforeEach, describe, expect, it, vi } from 'vitest';
import { useSignatureContext } from './SignatureProvider';
import { SignatureStatus } from './SignatureStatus';
import { useSignatureContext } from '../components/SignatureProvider';
import { SignatureStatus } from '../components/SignatureStatus';

vi.mock('./SignatureProvider', () => ({
vi.mock('../components/SignatureProvider', () => ({
useSignatureContext: vi.fn(),
}));
vi.mock('./SignatureLabel', () => ({
vi.mock('../components/SignatureLabel', () => ({
SignatureLabel: vi.fn(() => <div>SignatureLabel</div>),
}));

Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
import { fireEvent, render, screen } from '@testing-library/react';
import { type Mock, describe, expect, it, vi } from 'vitest';
import { MessageType } from '../types';
import { useSignatureContext } from './SignatureProvider';
import { SignatureToast } from './SignatureToast';
import { useSignatureContext } from '../components/SignatureProvider';
import { SignatureToast } from '../components/SignatureToast';

vi.mock('./SignatureProvider', () => ({
vi.mock('../components/SignatureProvider', () => ({
useSignatureContext: vi.fn(),
}));

vi.mock('./SignatureIcon', () => ({
vi.mock('../components/SignatureIcon', () => ({
SignatureIcon: vi.fn(() => <div>SignatureIcon</div>),
}));
vi.mock('./SignatureLabel', () => ({
vi.mock('../components/SignatureLabel', () => ({
SignatureLabel: vi.fn(() => <div>SignatureLabel</div>),
}));

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { describe, expect, it } from 'vitest';
import { MessageType } from '../types';
import { validateMessage } from './validateMessage';
import { validateMessage } from '../utils/validateMessage';

describe('validateMessage', () => {
it('should validate typed data message', () => {
Expand Down
Loading