-
Notifications
You must be signed in to change notification settings - Fork 76
Defaulter extension feature #827
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
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Hi! The link you provide in PR description: |
The PR I've linked is dependent on a forked devnet that has feature from this PR implemented (it will be merged soon). It also shows the implementation of a custom defaulter that uses sqllite pathfinder snapshot. Overall idea is to show you exact usecase for this feature. This PR is the first one that I need to push upstream to get rid of my devnet fork. |
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.
Did you imagine this as configurable via CLI?
As for the docs that need updating, please consider website/docs/forking.md
.
Regarding the tests, you may consult tests/integration/test_fork.rs
as an example, but since that file is already relatively big, you can consider adding a new testing file, similar to that one, but called e.g. test_fork_with_custom_defaulter.rs
@FabijanC I intentionally moved extension implementation from devnet to avoid bloating the depedencies and minimising changeset. It could be done though by adding sqlite (for example) implementation to the repo under the feature flag with static block that will register the extension, then everything should work out of the box using fork config url (I utilise url schema in to pick the right extension). But tbh my sqlite pathfinder snapshot usage is highly opinionated and might not work for everyone. Sqlite is also picked out of the simplicity, schema could be optimised to improve snapshot size. I would not want that bit to be provided as a "good default". Anyways that definitely should be added to devnet as a separate feature. Thank you for the directions. I'll provide tests and docs in the beginning of September. |
Ok, so it seems this doesn't change the CLI or the web-API for the user in any way (EDIT: ok, that's basically what you've written). I would normally describe this as a development-related change because CLI is the expected environment for using Devnet. I will change the PR description to mark your issue as closeable. |
Usage related changes
This PR allows users who utilize devnet programmatically (as a 3rd party crate) to provide their own implementation of defaulters. Thus allowing for example using Pathfinders sqlite state reader dramatically improving speed of operation.
Closes #828
PLEASE HELP:
Development related changes
Nothing really
Checklist:
./scripts/format.sh
./scripts/clippy_check.sh
-> ethers-providers cause tons of errors./scripts/check_unused_deps.sh
./scripts/check_spelling.sh
-> incorrect test caselatests
cause trouble./website/README.md