From f184d25cfc0cc5413ea34b9c0ea6533a71c8ef8a Mon Sep 17 00:00:00 2001 From: Dmytro Tolok Date: Wed, 30 Aug 2023 13:00:17 +0200 Subject: [PATCH] feat(perf): improvements (#852) * feat(perf): improvements * refactor(score): improve readability * refactor(getBlockNum): simplify logic * test(utils): change tests for modified files --- .husky/pre-commit | 1 + jest.config.unit.ts | 3 +- src/methods.test.ts | 165 ++++++++++++++++++++++++++++++ src/methods.ts | 12 +-- src/rpc.ts | 20 ++-- {test/unit => src}/scores.test.ts | 29 +++--- src/scores.ts | 24 ++--- src/utils.test.ts | 83 +++++++++++++++ src/utils.ts | 18 +++- 9 files changed, 305 insertions(+), 50 deletions(-) create mode 100644 src/methods.test.ts rename {test/unit => src}/scores.test.ts (76%) create mode 100644 src/utils.test.ts diff --git a/.husky/pre-commit b/.husky/pre-commit index 5a182ef1..796dcc7c 100755 --- a/.husky/pre-commit +++ b/.husky/pre-commit @@ -2,3 +2,4 @@ . "$(dirname -- "$0")/_/husky.sh" yarn lint-staged +yarn test:unit diff --git a/jest.config.unit.ts b/jest.config.unit.ts index ac9c3967..d736c70e 100644 --- a/jest.config.unit.ts +++ b/jest.config.unit.ts @@ -13,8 +13,7 @@ export default { preset: 'ts-jest', testEnvironment: 'jest-environment-node-single-context', moduleFileExtensions: ['js', 'ts'], - testMatch: ['/test/unit/**/?(*.)+(spec|test).(ts|js)'], - testPathIgnorePatterns: ['dist/', 'node_modules/'], + testMatch: ['/src/**/?(*.)+(spec|test).[jt]s?(x)'], verbose: true, globals: { 'ts-jest': { diff --git a/src/methods.test.ts b/src/methods.test.ts new file mode 100644 index 00000000..980ac268 --- /dev/null +++ b/src/methods.test.ts @@ -0,0 +1,165 @@ +import { getVp } from './methods'; +import snapshot from '@snapshot-labs/strategies'; +import { getBlockNum } from './utils'; +import * as redisModule from './redis'; + +jest.mock('@snapshot-labs/strategies'); +jest.mock('./utils'); +jest.mock('./redis', () => ({ + __esModule: true, + default: { + hGetAll: jest.fn(), + multi: jest.fn(() => ({ + hSet: jest.fn(), + exec: jest.fn() + })), + connect: jest.fn(), + on: jest.fn() + } +})); + +const mockRedis = redisModule.default; + +describe('getVp function', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should set snapshot to "latest" if it is not a number', async () => { + const expectedSnapshotNum = 'latest'; + const params = { + address: '0x123', + network: '1', + strategies: [], + snapshot: 'not-a-number', + space: 'testSpace' + }; + + (snapshot.utils.getVp as jest.Mock).mockResolvedValue({ vp_state: 'pending', vp: 100 }); + (getBlockNum as jest.Mock).mockResolvedValue(expectedSnapshotNum); + + // @ts-expect-error + await getVp(params); + + expect(getBlockNum).toHaveBeenCalledWith(expectedSnapshotNum, params.network); + expect(params.snapshot).toBe('latest'); + }); + + it('should call getBlockNum if snapshot is not "latest"', async () => { + const params = { + address: '0x123', + network: '1', + strategies: [], + snapshot: 1000, + space: 'testSpace' + }; + + (getBlockNum as jest.Mock).mockResolvedValue(900); + + await getVp(params); + + expect(getBlockNum).toHaveBeenCalledWith(1000, '1'); + }); + + it('should throw an error for disabled networks or spaces', async () => { + const params = { + address: '0x123', + network: '1319', + strategies: [], + snapshot: 12345, + space: 'testSpace' + }; + + try { + await getVp(params); + fail('Expected getVp to throw an error'); // This will fail the test if no error is thrown + } catch (error) { + expect(error).toMatch('something wrong with the strategies'); + } + }); + + it('should call snapshot.utils.getVp with correct parameters', async () => { + const params = { + address: '0x123', + network: '1', + strategies: [], + snapshot: 12345, + space: 'testSpace' + }; + + (snapshot.utils.getVp as jest.Mock).mockResolvedValue({ vp_state: 'pending', vp: 100 }); + (getBlockNum as jest.Mock).mockResolvedValue(params.snapshot); + + await getVp(params); + + expect(snapshot.utils.getVp).toHaveBeenCalledWith( + '0x123', + '1', + [], + 12345, + 'testSpace', + undefined + ); + }); + + it('should use cache if conditions are met', async () => { + const params = { + address: '0x123', + network: '1', + strategies: [], + snapshot: 1000, + space: 'testSpace' + }; + + (getBlockNum as jest.Mock).mockResolvedValue(900); + mockRedis.hGetAll.mockResolvedValue({ vp_state: 'pending', vp: 100, vp_by_strategy: '{}' }); + + const result = await getVp(params); + + expect(mockRedis.hGetAll).toHaveBeenCalled(); + expect(result.cache).toBe(true); + expect(result.result.vp).toBe(100); + }); + + it('should return correct result values', async () => { + const params = { + address: '0x123', + network: '1', + strategies: [], + snapshot: 1000, + space: 'testSpace' + }; + + mockRedis.hGetAll.mockResolvedValue(); + (snapshot.utils.getVp as jest.Mock).mockResolvedValue({ + vp_state: 'pending', + vp: 100, + vp_by_strategy: '{}' + }); + (getBlockNum as jest.Mock).mockResolvedValue(params.snapshot); + + const result = await getVp(params); + + expect(result.cache).toBe(false); + expect(result.result.vp).toBe(100); + expect(result.result.vp_state).toBe('pending'); + }); + + it('should handle the delegation parameter correctly', async () => { + const params = { + address: '0x123', + network: '1', + strategies: [], + snapshot: 1000, + space: 'testSpace', + delegation: true + }; + + (snapshot.utils.getVp as jest.Mock).mockResolvedValue({ vp_state: 'pending', vp: 100 }); + (getBlockNum as jest.Mock).mockResolvedValue(params.snapshot); + + await getVp(params); + + expect(snapshot.utils.getVp).toHaveBeenCalledWith('0x123', '1', [], 1000, 'testSpace', true); + }); +}); diff --git a/src/methods.ts b/src/methods.ts index 55a18cea..7e830668 100644 --- a/src/methods.ts +++ b/src/methods.ts @@ -21,31 +21,31 @@ interface ValidateRequestParams { params: any; } +const disabledNetworks = ['1319']; const disableCachingForSpaces = ['magicappstore.eth', 'moonbeam-foundation.eth']; export async function getVp(params: GetVpRequestParams) { if (typeof params.snapshot !== 'number') params.snapshot = 'latest'; - if (params.snapshot !== 'latest') { - const currentBlockNum = await getBlockNum(params.snapshot, params.network); - params.snapshot = currentBlockNum < params.snapshot ? 'latest' : params.snapshot; - } + params.snapshot = await getBlockNum(params.snapshot, params.network); const key = sha256(JSON.stringify(params)); const useCache = redis && params.snapshot !== 'latest' && !disableCachingForSpaces.includes(params.space); if (useCache) { const cache = await redis.hGetAll(`vp:${key}`); + if (cache?.vp_state) { cache.vp = parseFloat(cache.vp); + cache.vp_by_strategy = JSON.parse(cache.vp_by_strategy); return { result: cache, cache: true }; } } - if (['1319'].includes(params.network) || disabled.includes(params.space)) + if (disabledNetworks.includes(params.network) || disabled.includes(params.space)) { throw 'something wrong with the strategies'; - + } const result = await snapshot.utils.getVp( params.address, params.network, diff --git a/src/rpc.ts b/src/rpc.ts index 36df6c0d..32c87cc1 100644 --- a/src/rpc.ts +++ b/src/rpc.ts @@ -131,19 +131,13 @@ router.post('/api/scores', async (req, res) => { let result; try { - result = await scores( - { - requestId, - strategyNames - }, - { - space, - network, - snapshot, - strategies, - addresses - } - ); + result = await scores({ + space, + network, + snapshot, + strategies, + addresses + }); } catch (e: any) { capture(e, { params, strategies }); // @ts-ignore diff --git a/test/unit/scores.test.ts b/src/scores.test.ts similarity index 76% rename from test/unit/scores.test.ts rename to src/scores.test.ts index 4fe55eb0..0dbafd8d 100644 --- a/test/unit/scores.test.ts +++ b/src/scores.test.ts @@ -1,10 +1,10 @@ -import scores from '../../src/scores'; -import { get, set } from '../../src/aws'; -import { getBlockNum, sha256 } from '../../src/utils'; +import scores from './scores'; +import { get, set } from './aws'; +import { getBlockNum, sha256 } from './utils'; import snapshot from '@snapshot-labs/strategies'; -jest.mock('../../src/utils'); -jest.mock('../../src/aws'); +jest.mock('./utils'); +jest.mock('./aws'); jest.mock('@snapshot-labs/strategies'); describe('scores function', () => { @@ -25,8 +25,8 @@ describe('scores function', () => { it('should deduplicate requests', async () => { (snapshot.utils.getScoresDirect as jest.Mock).mockResolvedValue(mockScores); - const firstCall = scores(null, mockArgs); - const secondCall = scores(null, mockArgs); + const firstCall = scores(mockArgs); + const secondCall = scores(mockArgs); const firstResult = await firstCall; const secondResult = await secondCall; @@ -39,7 +39,7 @@ describe('scores function', () => { process.env.AWS_REGION = 'us-west-1'; (get as jest.Mock).mockResolvedValue(mockScores); - const result = await scores(null, mockArgs); + const result = await scores(mockArgs); expect(result).toEqual({ cache: true, @@ -54,16 +54,17 @@ describe('scores function', () => { (get as jest.Mock).mockResolvedValue(null); // Not in cache (snapshot.utils.getScoresDirect as jest.Mock).mockResolvedValue(mockScores); - await scores(null, mockArgs); + await scores(mockArgs); expect(set).toHaveBeenCalledWith(key, mockScores); }); it('should return uncached results when cache is not needed', async () => { process.env.AWS_REGION = 'us-west-1'; + (getBlockNum as jest.Mock).mockResolvedValue('latest'); (get as jest.Mock).mockResolvedValue(null); // Not in cache (snapshot.utils.getScoresDirect as jest.Mock).mockResolvedValue(mockScores); - const result = await scores(null, { ...mockArgs, snapshot: 'latest' }); // "latest" should bypass cache + const result = await scores({ ...mockArgs, snapshot: 'latest' }); // "latest" should bypass cache expect(result).toEqual({ cache: false, @@ -75,8 +76,9 @@ describe('scores function', () => { it("shouldn't return cached results when cache is not available", async () => { process.env.AWS_REGION = ''; + (getBlockNum as jest.Mock).mockResolvedValue(mockArgs.snapshot); (snapshot.utils.getScoresDirect as jest.Mock).mockResolvedValue(mockScores); - const result = await scores(null, mockArgs); + const result = await scores(mockArgs); expect(result).toEqual({ cache: false, @@ -88,9 +90,10 @@ describe('scores function', () => { it('should restrict block number by `latest`', async () => { (snapshot.utils.getScoresDirect as jest.Mock).mockResolvedValue(mockScores); - (getBlockNum as jest.Mock).mockResolvedValue('100'); + (getBlockNum as jest.Mock).mockResolvedValue('latest'); - const result = await scores(null, { ...mockArgs, snapshot: '99999999' }); + const args = { ...mockArgs, snapshot: '99999999' }; // block in the future + const result = await scores(args); expect(result).toEqual({ cache: false, diff --git a/src/scores.ts b/src/scores.ts index 81e9108b..9cc0a27c 100644 --- a/src/scores.ts +++ b/src/scores.ts @@ -5,29 +5,29 @@ import serve from './requestDeduplicator'; const broviderUrl = process.env.BROVIDER_URL || 'https://rpc.snapshot.org'; -async function calculateScores(parent, args, key) { +async function calculateScores(args, key) { const withCache = !!process.env.AWS_REGION; const { space = '', strategies, network, addresses } = args; - let snapshotBlockNum = 'latest'; - - if (args.snapshot !== 'latest') { - const currentBlockNum = await getBlockNum(args.snapshot, network); - snapshotBlockNum = currentBlockNum < args.snapshot ? 'latest' : args.snapshot; - } + const snapshotBlockNum = await getBlockNum(args.snapshot, network); const state = snapshotBlockNum === 'latest' ? 'pending' : 'final'; + let scores; + let cache = false; - if (withCache && state === 'final') scores = await get(key); + if (withCache && state === 'final') { + cache = true; + scores = await get(key); + } - let cache = true; if (!scores) { cache = false; + const provider = snapshot.utils.getProvider(network, { broviderUrl }); scores = await snapshot.utils.getScoresDirect( space, strategies, network, - snapshot.utils.getProvider(network, { broviderUrl }), + provider, addresses, snapshotBlockNum ); @@ -44,8 +44,8 @@ async function calculateScores(parent, args, key) { }; } -export default function scores(parent, args) { +export default function scores(args) { const id = JSON.stringify(args); const cacheKey = sha256(id); - return serve(id, calculateScores, [parent, args, cacheKey]); + return serve(id, calculateScores, [args, cacheKey]); } diff --git a/src/utils.test.ts b/src/utils.test.ts new file mode 100644 index 00000000..04adec03 --- /dev/null +++ b/src/utils.test.ts @@ -0,0 +1,83 @@ +const originalBroviderUrl = process.env.BROVIDER_URL; +process.env.BROVIDER_URL = 'test.brovider.url'; + +import { getBlockNum, blockNumByNetwork } from './utils'; +import snapshot from '@snapshot-labs/strategies'; + +jest.mock('@snapshot-labs/strategies'); +jest.mock('./utils', () => { + const originalModule = jest.requireActual('./utils'); + return { + ...originalModule, + getBlockNum: jest.fn(originalModule.getBlockNum) + }; +}); + +describe('getBlockNum function', () => { + beforeEach(() => { + jest.clearAllMocks(); + Object.keys(blockNumByNetwork).forEach((key) => delete blockNumByNetwork[key]); + }); + + afterAll(() => { + process.env.BROVIDER_URL = originalBroviderUrl; + }); + + it('should return "latest" if snapshotBlock is "latest"', async () => { + const result = await getBlockNum('latest', '1'); + expect(result).toBe('latest'); + }); + + it('should return block number from blockNumByNetwork if it exists and is less than or equal to snapshotBlock', async () => { + const firstRequestBlockNum = 100; + const secondRequestBlockNum = 99; + const mockProvider = { + getBlockNumber: jest.fn().mockResolvedValue(firstRequestBlockNum) + }; + (snapshot.utils.getProvider as jest.Mock).mockReturnValue(mockProvider); + await getBlockNum(firstRequestBlockNum, '1'); + + const result = await getBlockNum(secondRequestBlockNum, '1'); + expect(result).toBe(firstRequestBlockNum); + }); + + it('should return block number from blockNumByNetwork if it exists and is greater than snapshotBlock and timestamp is within delay', async () => { + const firstRequestBlockNum = 100; + const secondRequestBlockNum = 101; + + const mockProvider = { + getBlockNumber: jest.fn().mockResolvedValue(firstRequestBlockNum) + }; + (snapshot.utils.getProvider as jest.Mock).mockReturnValue(mockProvider); + await getBlockNum(firstRequestBlockNum, '1'); + + const result = await getBlockNum(secondRequestBlockNum, '1'); + expect(result).toBe(firstRequestBlockNum); + }); + + it('should fetch block number from provider if not in blockNumByNetwork or timestamp is beyond delay', async () => { + const mockBlockNumber = '120'; + const mockProvider = { + getBlockNumber: jest.fn().mockResolvedValue(mockBlockNumber) + }; + (snapshot.utils.getProvider as jest.Mock).mockReturnValue(mockProvider); + + const result = await getBlockNum(110, '1'); + expect(result).toBe(110); + expect(snapshot.utils.getProvider).toHaveBeenCalledWith('1', { + broviderUrl: process.env.BROVIDER_URL + }); + expect(mockProvider.getBlockNumber).toHaveBeenCalled(); + }); + + it('should return "latest" if fetched block number is less than snapshotBlock', async () => { + const mockBlockNumber = 90; + const mockProvider = { + getBlockNumber: jest.fn().mockResolvedValue(mockBlockNumber) + }; + (snapshot.utils.getProvider as jest.Mock).mockReturnValue(mockProvider); + + const result = await getBlockNum(110, '1'); + expect(result).toBe('latest'); + }); +}); diff --git a/src/utils.ts b/src/utils.ts index 15d36b9a..afff36f9 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -59,12 +59,22 @@ export function rpcError(res, code, e, id) { }); } -export async function getBlockNum(snapshotBlock, network) { - if (blockNumByNetwork[network] && snapshotBlock <= blockNumByNetwork[network]) +type SnapshotBlockType = number | 'latest'; +export async function getBlockNum(snapshotBlock: SnapshotBlockType = 'latest', network) { + if (snapshotBlock === 'latest') { + return snapshotBlock; + } + + // TODO: if a user pass a block number in the future, we always try to get the latest block number + if (blockNumByNetwork[network] && snapshotBlock <= blockNumByNetwork[network]) { return blockNumByNetwork[network]; + } + const ts = parseInt((Date.now() / 1e3).toFixed()); - if (blockNumByNetwork[network] && blockNumByNetworkTs[network] > ts - delay) + + if (blockNumByNetwork[network] && blockNumByNetworkTs[network] > ts - delay) { return blockNumByNetwork[network]; + } const provider = snapshot.utils.getProvider(network, { broviderUrl }); const blockNum = await provider.getBlockNumber(); @@ -72,7 +82,7 @@ export async function getBlockNum(snapshotBlock, network) { blockNumByNetwork[network] = blockNum; blockNumByNetworkTs[network] = ts; - return blockNum; + return blockNum < snapshotBlock ? 'latest' : snapshotBlock; } export function getIp(req) {