Skip to content

feat: add miner extension trait#325

Merged
mattsse merged 2 commits intomainfrom
matt/add-op-miner-extension
Dec 3, 2024
Merged

feat: add miner extension trait#325
mattsse merged 2 commits intomainfrom
matt/add-op-miner-extension

Conversation

@mattsse
Copy link
Copy Markdown
Member

@mattsse mattsse commented Dec 3, 2024

Comment on lines +140 to +141
#[cfg_attr(not(feature = "client"), rpc(server, namespace = "miner"))]
#[cfg_attr(feature = "client", rpc(server, client, namespace = "miner"))]
Copy link
Copy Markdown
Collaborator

@refcell refcell Dec 3, 2024

Choose a reason for hiding this comment

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

Curious, is it destructive to do the following instead?

Suggested change
#[cfg_attr(not(feature = "client"), rpc(server, namespace = "miner"))]
#[cfg_attr(feature = "client", rpc(server, client, namespace = "miner"))]
#[rpc(server, namespace = "miner")]
#[cfg_attr(feature = "client", rpc(client))]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

iirc this didn't quite work, but can't remember specifics

@mattsse mattsse added this pull request to the merge queue Dec 3, 2024
Merged via the queue into main with commit 493af3a Dec 3, 2024
@mattsse mattsse deleted the matt/add-op-miner-extension branch December 3, 2024 20:38
github-merge-queue bot pushed a commit that referenced this pull request Dec 17, 2024
<!--
Thank you for your Pull Request. Please provide a description above and
review
the requirements below.

Bug fixes and new features should include tests.

Contributors guide:
https://github.com/alloy-rs/core/blob/main/CONTRIBUTING.md

The contributors guide includes instructions for running rustfmt and
building the
documentation.
-->

<!-- ** Please select "Allow edits from maintainers" in the PR Options
** -->

## Motivation

op batcher prints error
`ERROR[12-17|18:17:23.568] Result of SetMaxDASize was false, retrying.`

command for op-bathcer
```shell
op-batcher \
  --l2-eth-rpc=http://localhost:8545 \
  --rollup-rpc=http://localhost:9545 \
  --poll-interval=1s \
  --sub-safety-margin=6 \
  --num-confirmations=1 \
  --safe-abort-nonce-too-low-count=3 \
  --resubmission-timeout=30s \
  --rpc.addr=0.0.0.0 \
  --rpc.port=8548 \
  --rpc.enable-admin \
  --max-channel-duration=25 \
  --l1-eth-rpc=$L1_RPC_URL \
  --private-key=$GS_BATCHER_PRIVATE_KEY
```

golang implementation of optimism's op-geth [returns
bool](https://github.com/ethereum-optimism/op-geth/blob/optimism/eth/api_miner.go#L62)
type for `miner_setMaxDASize` call. Therefore, expected response might
be like following:

```json
{
  "jsonrpc": "2.0",
  "id": 0,
  "result": true
}
```

On the other hand, current trait [returns
`()`](https://github.com/alloy-rs/op-alloy/blob/main/crates/rpc-jsonrpsee/src/traits.rs#L146),
represents null for json.

```rust
pub trait MinerApiExt {
    /// Sets the maximum data availability size of any tx allowed in a block, and the total max l1
    /// data size of the block. 0 means no maximum.
    #[method(name = "setMaxDASize")]
    async fn set_max_da_size(&self, max_tx_size: U64, max_block_size: U64) -> RpcResult<()>; // should return bool
}
```

```json
{
  "jsonrpc": "2.0",
  "id": 0,
  "result": null
}
```

ref: #325, #345

## Solution

alter `()` to `bool` for return type

```rust
async fn set_max_da_size(&self, max_tx_size: U64, max_block_size: U64) -> RpcResult<bool>;
```

## PR Checklist

- [ ] Added Tests
- [ ] Added Documentation
- [x] Breaking changes
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.

2 participants