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(anvil): add anvil_getIntervalMining API #9290

Merged
merged 3 commits into from
Nov 9, 2024

Conversation

feynman-x
Copy link
Contributor

Motivation

Closes #8813

Solution

@feynman-x
Copy link
Contributor Author

@grandizzy Followed your suggestion and created this new PR (old #9090). Please check if there is anything to improve.

Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! Just a small nit, IMO we should be consistent naming at par with anvil_setIntervalMining e.g. anvil_getIntervalMining (instead anvil_getIntervalMine)

crates/anvil/src/eth/api.rs Outdated Show resolved Hide resolved
crates/anvil/src/eth/api.rs Outdated Show resolved Hide resolved
crates/anvil/core/src/eth/mod.rs Outdated Show resolved Hide resolved
@grandizzy grandizzy self-requested a review November 9, 2024 15:29
@feynman-x
Copy link
Contributor Author

@grandizzy Yes, it's reasonable. The PR has been updated.

Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

Lgtm, thank you! Pending additional review before merging.

@grandizzy grandizzy changed the title feat: add anvil_get_interval_mine method feat(anvil): add anvil_getIntervalMining API Nov 9, 2024
@feynman-x
Copy link
Contributor Author

Thanks for your guidance. I'm so excited, this is my first PR for Foundry. Hope to contribute more in the future.

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

great, this is a lot simpler

@mattsse mattsse merged commit 9df5939 into foundry-rs:master Nov 9, 2024
21 checks passed
/// Returns the value of mining interval, if set.
///
/// Handler for ETH RPC call: `anvil_getIntervalMing`
pub fn anvil_get_interval_ming(&self) -> Result<Option<u64>> {
Copy link

@holic holic Nov 9, 2024

Choose a reason for hiding this comment

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

should this be anvil_get_interval_mining not anvil_get_interval_ming? (and the comment above)

Copy link
Collaborator

Choose a reason for hiding this comment

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

thank you! yes indeed, missed the typo 😁 preparing an update in #9292

@feynman-x feynman-x deleted the feat/anvil_interval_mining_v2 branch November 20, 2024 06:47
rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
* feat: add anvil_get_interval_mine method

* refactor: keep consistent naming

---------

Co-authored-by: Your Name <you@example.com>
Co-authored-by: grandizzy <38490174+grandizzy@users.noreply.github.com>
@grandizzy grandizzy added T-feature Type: feature C-anvil Command: anvil labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-anvil Command: anvil T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(anvil): add anvil_getIntervalMining method or make anvil_setAutomine resume interval mining
4 participants