feat: arbitrage-agent #11
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0xJepsen
reviewed
Sep 19, 2023
0xJepsen
reviewed
Sep 19, 2023
0xJepsen
reviewed
Sep 19, 2023
0xJepsen
reviewed
Sep 19, 2023
0xJepsen
reviewed
Sep 19, 2023
Autoparallel
requested review from
zachdt,
kinrezC,
Alexangelj,
clemlak and
0xJepsen
0xJepsen
suggested changes
Sep 21, 2023
| function swap(address token, uint256 amount) external; | ||
| } | ||
|
|
||
| interface TokenLike { |
There was a problem hiding this comment.
Could we inherent an erc standard here instead?
There was a problem hiding this comment.
Yeah we could replace TokenLike with ERC20 probably. @Alexangelj ?
Alexangelj
reviewed
Sep 22, 2023
There was a problem hiding this comment.
I have a few comments:
- I don't see any tests, could we have tests made when we have big functionality like this?
- The agent is responsible for its own smart contract, executing logic against the smart contract, and hooking into the sim. These are all things I think my box-core can eventually be responsible for, but for now this works and it's simple
- Really hate committing bindings
On the bindings, sort of unrelated to this pr and dont expect us to make this for this pr but an idea I just thought of:
- builder pattern for a smart contract (i.e. exchange) in rust, where we have a method "add_function" and it will add the respective abi to the restultant smart contract object. Then we don't need the bindings all the time, we just explicitly define the abi for a method (could use alloy-core maybe for that), then add it via the builder pattern. We still get self.exchange.status() or whatever method without worrying about bindings.
…tivefinance/portfolio-in-a-box into colin/feat/arbitrage-agent
0xJepsen
approved these changes
Oct 5, 2023
Alexangelj
requested changes
Oct 5, 2023
There was a problem hiding this comment.
This looks like its branched off @clemlak 's branch, can we change the target to that branch or block this from being merged until that's in first?
|
@Alexangelj seems like Clement's branch got in and now this one is updated. Should be good to go. |
Alexangelj
approved these changes
Oct 10, 2023
|
Need review from @kinrezC |
kinrezC
approved these changes
Oct 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Merge after #21
Aiming to close #1.
Currently: