-
Notifications
You must be signed in to change notification settings - Fork 39
feat: Simulate txs against RPC #261
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
Conversation
ae5f009 to
2dce6e7
Compare
2dce6e7 to
d967dcf
Compare
dianacarvalho1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uau thank you @tamaralipows 🙏🏼 what a hard task!! I only did a general review and left some comments mostly for my understanding! When you start we can meet about this and run it together!
We should probably fetch this from execution instead. Will do later
71ab610 to
a7f3234
Compare
# Conflicts: # protocol-testing/Cargo.lock # protocol-testing/Cargo.toml # protocol-testing/entrypoint.sh # protocol-testing/src/encoding.rs # protocol-testing/src/test_runner.rs #time 5h 45m
Decoded amount out from execution and compared with the one from simulation Allow for printing the execution traces Moved all execution methods into execution.rs Created traces.rs (heavily clauded) that prints the traces nicely and gets the function selectors frm 4byte (I decided not to use EtherscanIdentifier from foundry because adding foundry as a dependency is really heavy and I expect problems because of it) Temporary: hardcoded storage slots for test_weighted_pool_v4 to pass #time 1h 55m
…hereum #time 1h 14m
#time 2m
tamaralipows
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all of this! I'm happy with this!
Regarding your comments:
I think we should just use an executor address that is already approved and mock the executor code to our current executor instead of checking if the current one is approved as well.
Isn't this more difficult and slightly disorganized, requiring us to overwrite the executor address in the execution repo? Not sure how you would do this in a clean way
I just ran this only for test_weighted_pool_v4 and for the case of the biggest amount in, the swap fails - I dunno why yet.
Oh please no not this issue again lol. I can help you debug this tomorrow
Should we add another flag to show the execution traces? I just reused the vm_traces but it's not really the same thing. And most likely the user will only want to see one or the other
I don't fully understand why the user would want to see vm_traces that are not execution related, without seeing the execution traces? Maybe we can just call this "traces"?
|
Thank you for the review @tamaralipows 🙏🏼 to answer your comments:
But then we rely on the executor being actually deployed on chain no? 🤔 and the integrators would like to run this test way before that 😬
Ah, originally the |
Also get tycho router bytecode at compile time #time 22m
@dianacarvalho1 No, why do you say that? I have added code to overwrite the executor code and also the executor approval on the tycho router 🤔 |
Remove unnecessary TODOs #time 35m
#time 41m #time 0m
f1f6f86 to
9c3ee07
Compare
Improved the format of the printed trace as well #time 1h 7m
adrianbenavides
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Craaazy piece of work! I did a superficial pass and I left some minor comments/questions. I want to do another pass tomorrow for my own good, but feel free to merge it when you see fit.
# Conflicts: # protocol-testing/src/test_runner.rs #time 7m
#time 3m
tamaralipows
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I can't approve because it's my PR but it all looks good, I only have really minor comments!
| .join(",") | ||
| ); | ||
|
|
||
| // Filter out scam/honeypot signatures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats a honeypot signature? I know you didn't just add this now but I'm curious now hahaha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"A honeypot is a trap designed to attract and catch someone or something, originally referring to using honey to catch bears or other animals."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the term but I mean practically what does that mean in this context and why are we filtering it out xD don't we want to know if we are being scammed
Add trying to decode method's calldatas in tracing #time 1m
… method: bytes_to_fixed_32 #time 13m
# Conflicts: # protocol-testing/src/test_runner.rs #time 2m
#time 1m
Main points:
debug_traceCalland prints traces ifvm_tracesis set to true.I am running the old-fashioned way using:
cargo run -- --package ethereum-balancer-v2TODO (not all necessarily in this PR):
Questions:
Example logs: