Skip to content

Conversation

@itaven
Copy link

@itaven itaven commented Nov 12, 2025

Description

Add Playwright test template for CLI.

  • Added initial test template for CLI-based tests
  • Added GitHub Actions workflow to run tests on PRs
  • Added README.md with setup and usage instructions
  • Created and initialized Playwright projects:
    • lsv_cli_tests and setup lsv_cli before all
    • setup lsv_cli runs once before all tests to configure the defaultVault, preventing repeated vault creation and granting required roles for test reuse
  • Created a reusable defaultVault instance with all permissions granted via the grantRole function
  • Test setup is decoupled from CLI setup:
    • Only CHAIN_ID needs to be set; the suite uses a free public RPC with an optional override via RPC_URL in .env
    • Designed for scalability and potential mainnet testing
  • Moved utils (previously in tests/utils) → unit directory
  • Added base test for vault creation (connected to Hub in globalSetup) and a demo supply test using defaultVault

Code Review Notes

  • Pay attention to the lint-ignore assertion here.
    This is intentional because process.env.VAULT_ADDRESS and dashboardAddress: process.env.DASHBOARD_ADDRESS are set dynamically in globalSetup.ts.
    Since Playwright projects run in separate processes, environment variables (even static ones) can’t be shared.
    To handle this, a sub-project runs once before all tests to establish shared state, and is not executed again afterwards.

  • Review the naming for the base directory — it was created to separate existing unit tests.
    We might consider renaming basee2e to better reflect its purpose and keep a clear distinction from unit tests.

  • Should we keep 1 wf file for checks or stay with separated checks && tests

  • tests/unitare currently executed via the rootpackage.jsonusingyarn test` (Jest).
    We should decide whether to:

    1. Keep Playwright tests isolated and executed only from tests/base/package.json, or
    2. Link Playwright execution to the root test workflow (for example, via a script alias like yarn test:e2e).

    The first option keeps responsibilities clear (unit vs e2e), while the second simplifies CI integration if we want a unified yarn test entry point.


Checklist

  • Checked the changes locally
  • Created/updated unit tests
  • Created/updated README.md

@itaven itaven requested a review from a team as a code owner November 12, 2025 14:20
[
ROLES.DEFAULT_ADMIN,
{
index: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

why you dont use the index from Array?

Copy link
Author

@itaven itaven Nov 20, 2025

Choose a reason for hiding this comment

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

I could build the map dynamically, but I don’t yet know the canonical way
to derive a role’s keccak from roleName .I tried a few approaches, but the keccak I compute still doesn’t match the contract’s value. If you point me to correct keccak function I can do like this:

const rolesArray = Object.values(ROLES);

export const PERMISSION_ROLES = new Map(
  rolesArray.map((role, index) => [
    role,
    {
      index,
      keccak: utils.keccak256(utils.toUtf8Bytes(role)),
    },
  ]),
);

Copy link
Contributor

Choose a reason for hiding this comment

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

if you want create keccak dinamaly, you can use like this:

import { keccak256, toHex } from 'viem';

const toRoleHash = (role: string) => keccak256(toHex(role));

export const PERMISSIONS_FUND_ROLE = 'vaults.Permissions.Fund';
const PERMISSIONS_FUND_ROLE_KECCAK = toRoleHash(PERMISSIONS_FUND_ROLE);

But my question was why you need to store the index of an element in an array if the array itself already has one.
array.map((item, index <---) => {})

Copy link
Author

Choose a reason for hiding this comment

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

Initially, I couldn’t iterate the array and use the index directly because I didn’t have a way to compute the keccak hash on the fly.

In my case I couldn’t just iterate with
array.map((item, index) => …) and compute keccak(item) on the fly, because I have a couple of additional roles that don’t fit into the simple “keccak of role name” pattern (for example, DEFAULT_ADMIN='0x000000' and an extra STRANGER role, VAULT_OWNER_AND_NoMANAGER)

So I ended up going with a small lookup table of precomputed hashes instead:

const ROLE_KECCAKS: Record<RoleValue, Hex> = {
  // e.g.
  // DEFAULT_ADMIN: '0x...',
  // STRANGER: '0x...',
  // NODE_OPERATOR: '0x...',
  // ...
};

export const PERMISSION_ROLES = new Map<RoleValue, RoleData>(
  Object.values(ROLES).map((role, index) => [
    role,
    {
      index,
      keccak: ROLE_KECCAKS[role],
    },
  ]),
);

This way I still get a nice indexed PERMISSION_ROLES map, but I’m not forced to derive every role’s hash in the same way — special cases like DEFAULT_ADMIN are handled naturally through the ROLE_KECCAKS mapping.

defaultAdmin: roles.defaultAdmin.address,
nodeOperator: roles.nodeOperator.address,
nodeOperatorManager: roles.nodeOperatorManager.address,
confirmExpiry: 86_400,
Copy link
Contributor

Choose a reason for hiding this comment

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

86400/100 - move to constants

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, but 86400 is correct value in seconds

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant to move both numbers into constants 86400 and 100

Copy link
Author

Choose a reason for hiding this comment

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

Already fixed

});
});

// Look out "🧑‍🔧 Debugging Tip" point for debug in Readme.md
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could control the display of logs by configuring test runs rather than commenting them out in the code.

Copy link
Author

@itaven itaven Nov 20, 2025

Choose a reason for hiding this comment

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

This isn’t a test log but a quick debugging tip: during debug it's good point how to not execute globalBeforeAll every time. Just thought it can be usable. That’s why we point to the "Debugging Tip" section here.

Just removed it. The debug tip is still in README.md. Commit

Copy link
Contributor

Choose a reason for hiding this comment

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

You can display specific logs depending on the log level passed, instead of manually writing them in the right places or commenting them each time

Copy link
Author

Choose a reason for hiding this comment

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

Got you, but anyway i removed it from comments. Currently stored only in README.md

"@lidofinance/wallets-testing-wallets": "^1.39.0",
"@playwright/test": "^1.56.1",
"viem": "^2.39.3",
"zod": "^3.24.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need zod?

Copy link
Author

@itaven itaven Dec 1, 2025

Choose a reason for hiding this comment

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

Initially, I used it for several required env variables, but in this case I can use it without Zod.
Removed.

# Conflicts:
#	package.json
#	tests/unit/bigInt.test.ts
#	tests/unit/calculate-health.test.ts
#	tests/unit/calculate-overview-v2.test.ts
#	tests/unit/csv-file.test.ts
#	tests/unit/lido-apr.test.ts
#	tests/unit/merkle-utils.test.ts
#	tests/unit/rebase-rewards.test.ts
#	tests/unit/snake-to-camel.test.ts
#	tests/unit/timestamp.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants