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

feat: add mellow-vaults bot scaffold #555

Open
wants to merge 63 commits into
base: main
Choose a base branch
from
Open

Conversation

katamarinaki
Copy link
Contributor

@katamarinaki katamarinaki commented Jun 2, 2024

Alerts for Mellow vaults

katamarinaki and others added 4 commits June 3, 2024 00:36
limits integrity (mellow-vaults)
wstETH integrity(wstETH integrity)
admin msig (multisig-watcher)
AccessControl (multisig-watcher, ethereum-governance, multisig-watcher)
mellow-vaults/src/shared/constants/common/mainnet.ts Outdated Show resolved Hide resolved
mellow-vaults/src/shared/events/acl_events.ts Outdated Show resolved Hide resolved
mellow-vaults/src/shared/notice.ts Outdated Show resolved Hide resolved
mellow-vaults/tests/e2e/vault-changes.e2e-spec.ts Outdated Show resolved Hide resolved
mellow-vaults/package.json Outdated Show resolved Hide resolved
mellow-vaults/README.md Outdated Show resolved Hide resolved
mellow-vaults/jest.config-e2e.json Outdated Show resolved Hide resolved
mellow-vaults/tests/e2e/utils.ts Outdated Show resolved Hide resolved
multisig-watcher/package.json Outdated Show resolved Hide resolved
multisig-watcher/tsconfig.json Outdated Show resolved Hide resolved
Copy link
Contributor Author

@katamarinaki katamarinaki left a comment

Choose a reason for hiding this comment

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

Please take a look at the review comments!

mellow-vaults/README.md Outdated Show resolved Hide resolved
mellow-vaults/src/clients/eth_provider.ts Outdated Show resolved Hide resolved
mellow-vaults/src/clients/eth_provider.ts Outdated Show resolved Hide resolved
mellow-vaults/src/clients/eth_provider.ts Outdated Show resolved Hide resolved
mellow-vaults/src/app.ts Outdated Show resolved Hide resolved
mellow-vaults/src/agent.ts Outdated Show resolved Hide resolved
mellow-vaults/src/agent.ts Outdated Show resolved Hide resolved
mellow-vaults/src/agent.ts Show resolved Hide resolved
mellow-vaults/src/agent.ts Outdated Show resolved Hide resolved
mellow-vaults/src/app.ts Outdated Show resolved Hide resolved
mellow-vaults/src/utils/events/safe_events.ts Outdated Show resolved Hide resolved
mellow-vaults/src/utils/events/safe_events.ts Outdated Show resolved Hide resolved
mellow-vaults/src/utils/events/withdrawals_events.ts Outdated Show resolved Hide resolved
mellow-vaults/src/utils/events/withdrawals_events.ts Outdated Show resolved Hide resolved
mellow-vaults/src/utils/events/withdrawals_events.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@sergeyWh1te sergeyWh1te left a comment

Choose a reason for hiding this comment

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

Main ideas where I'd like to see technical improvements.

  1. Network. Making network calls to handle transactions. If you debug the handleTransaction you can find that this method passes though itself a thousands transactions of the block.
  2. Local infra:
    1. Tests: They would not work in local infra. (Because it requires changing function's signature handleBlock, handleTx) I don't know do you or not roll out this bot in local env.
    2. Bot does not prepared for local infra.
  3. Found some duplicated pieces of the code
  4. Some variables are good to see as dependencies, not just consts. I see that you're planning to run bot in testnet. It means that constants would have different values. In future, you need to refactor this. Let's do it from the begging.
  5. Tests:
    I'd be perfect to cover by tests network layer, especially when code grabs wide ranges of blocks like - HOURS_48
  6. Types. I would strongly adhere to the strict types. Not mixing js and ts approaches.

Ton of work.
Let's hit the gas into the floor and do it perfectly. 💪🚀

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.

4 participants