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

Prepare bot for public use #1

Merged
merged 30 commits into from
Jun 26, 2024
Merged

Prepare bot for public use #1

merged 30 commits into from
Jun 26, 2024

Conversation

aquarat
Copy link
Contributor

@aquarat aquarat commented Jun 17, 2024

This PR is largely a copy of the orbit-specific OEV searcher bot, but with the following removed:

  • extraneous dependencies
  • typechain
  • contract compilation added back for the Liquidator (to enabled verifiable deployment by the user)
  • Jest, tests, e2e tests
  • Release/publishing/tagging scripts
  • Renovate

Functions have had documentary comments added and the README has been expanded.

The Orbit Liquidator contract's bytecode is included in the cli-utils file to enable deployment of that contract.

There is some code to optionally persist accounts to watch (to file) - which may not be desirable 🤷 (Update: this will be addressed in a separate PR)

It's a bit icky but a lot of the recent changes in oev-searcher will probably have to be downstreamed into this repo (in a separate PR).

There are a few temporary links pointing to the draft OEV docs. These will need to be updated once those docs are live.

Closes https://github.com/api3dao/oev-searcher/issues/17

@aquarat aquarat changed the title Initial trimming and refactoring Prepare bot for public use Jun 20, 2024
@aquarat aquarat marked this pull request as ready for review June 20, 2024 11:03
Copy link
Collaborator

@Siegrift Siegrift left a comment

Choose a reason for hiding this comment

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

👍 LGTM overall, just had a few minor comments.

I wonder if the recent patches to the internal OEV bot won't cause issues, but let's see. I'm hopeful it should be managaeble. If not, we can open source this as is. The only prerequisite is to make sure it is working.

.env.example Outdated Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
renovate.json Outdated Show resolved Hide resolved
src/accounts-to-watch.json.ignore.example Outdated Show resolved Hide resolved
src/cli-utils.ts Outdated Show resolved Hide resolved
src/interfaces.ts Outdated Show resolved Hide resolved
src/oev-seeker.ts Outdated Show resolved Hide resolved
@Siegrift Siegrift requested a review from andreogle June 24, 2024 12:23
Copy link
Member

@andreogle andreogle left a comment

Choose a reason for hiding this comment

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

I think there is a LOT of extra stuff here that shouldn't be included for an "example" repo that adds to what a reader needs to understand. e.g. console.log instead of logger, validating stuff with zod, sanitising ethers errors, ProviderWithFallback/other fallbacks, using immer instead of just a simple object etc.

In my mind, the focus should be on simplicity & readability as the main priorities so that potential searchers can understand the flow here. I really struggled to understand what was going on after a few functions. Maybe I'm misunderstanding the goal here, but I think there should be two repos:

  1. The "example" bot which focuses on being the absolute minimum required to get up and running with OEV. It doesn't do fallbacks, fancy logging, validation etc.
  2. The more advanced bot with all of these nice to have features that is more efficient/resilient.

.github/workflows/main.yml Outdated Show resolved Hide resolved

This repository contains an example OEV Searcher bot implementation targeting [Orbit Lending](https://orbitlending.io/).
To understand how OEV works, visit
[the OEV documentation](https://oev-docs--pr12-new-oev-docs-0y2wddya.web.app/reference/oev-network/overview/oev-network.html).
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to remember to update this

Copy link
Collaborator

Choose a reason for hiding this comment

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

We will in the following PR. The docs are WIP as of now.

- Set the value of the target beacon to our transmuted value: `updateBeaconWithSignedData`
- In a single call, apply the transmutation call data and retrieve account liquidity for all accounts
- refer to
[ExternalMulticallSimulator.sol](https://github.com/api3dao/oev-searcher/blob/main/contracts/api3-contracts/utils/ExternalMulticallSimulator.sol)
Copy link
Member

Choose a reason for hiding this comment

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

this repo is private so the link won't work. I'm not sure what the plans to make it public are though (if there are any)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be linked from the main branch of api3/contracts once this is done

contracts/Lock.sol Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/accounts-to-watch.json.ignore.example Outdated Show resolved Hide resolved
src/accounts-to-watch.ts Outdated Show resolved Hide resolved
src/accounts-to-watch.ts Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@Siegrift Siegrift left a comment

Choose a reason for hiding this comment

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

I think there is a LOT of extra stuff here that shouldn't be included for an "example" repo that adds to what a reader needs to understand. e.g. console.log instead of logger, validating stuff with zod, sanitising ethers errors, ProviderWithFallback/other fallbacks, using immer instead of just a simple object etc.

Yeah, I think these are valid simplifications. I've asked @aquarat not to make many code changes though, because we had some more changes+fixes planned in the oev-searcher repo. The plan should be:

  1. Port the oev-bot code from the oev-searcher repo to this example bot
  2. Implement simplifications, e.g. the ones you pointed out

In my mind, the focus should be on simplicity & readability as the main priorities so that potential searchers can understand the flow here. I really struggled to understand what was going on after a few functions. Maybe I'm misunderstanding the goal here, but I think there should be two repos:
The "example" bot which focuses on being the absolute minimum required to get up and running with OEV. It doesn't do fallbacks, fancy logging, validation etc.
The more advanced bot with all of these nice to have features that is more efficient/resilient.

That is too much work for little benefit. Imo, we should just have a single working open source implementation showcasing the OEV bot. Any resiliency is just extra complexity and something the user should implement on their own (and we don't need to provide that).

@aquarat
Copy link
Contributor Author

aquarat commented Jun 25, 2024

In general I'll try and simplify this much further and then you both can re-review.
There's also the question of how to handle the contracts (as per comment #1 (comment) )

@andreogle
Copy link
Member

Thanks 👍🏻 I'm fine with follow up PRs too btw

single working open source implementation showcasing the OEV bot

Ok but what is the focus of the "single working open source implementation"? Is it for educating new/potential searchers or is it something anyone can plug & play to compete for OEV (even if there are lots of people running the exact same code)? Or maybe it's the exact OEV bot that API3 is running for everyone to see and outcompete

These are different goals to me and strongly affect the direction of what the code will look like. i.e. Should the code be barebones/minimal or should it have lots of extra things to make it more efficient?

@aquarat
Copy link
Contributor Author

aquarat commented Jun 25, 2024

Good questions. It's probably worth discussing on Slack? 🤷
There's also this repo: https://github.com/api3dao/oev-searcher-starter

src/accounts-to-watch.ts Show resolved Hide resolved
Copy link

@vponline vponline left a comment

Choose a reason for hiding this comment

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

Looking great, special thanks for the transmutation details, it was very helpful for me 😄

@aquarat
Copy link
Contributor Author

aquarat commented Jun 26, 2024

This is not perfect, but I think it's at the point where others can assist, especially Andre.
I'll merge so long.
This PR now integrates the new contract but not all code calls it (some of it still calls the Orbit instance).

@aquarat aquarat merged commit d31d8fb into main Jun 26, 2024
2 checks passed
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