feat(rpc): add eth_coinbase implementation#1597
feat(rpc): add eth_coinbase implementation#1597MegaRedHand wants to merge 2 commits intoparadigmxyz:mainfrom
eth_coinbase implementation#1597Conversation
Co-authored-by: lambdaclass-user <github@lambdaclass.com>
Co-authored-by: lambdaclass-user <github@lambdaclass.com>
|
I was unsure of where to put the coinbase configuration. I assume the transaction pool would be in charge of bundling new transactions and minting new blocks (albeit delegating execution), so I think this could be a good place to store it. wdyt? |
| /// The address of the pool owner. | ||
| pub coinbase: Address, |
There was a problem hiding this comment.
hmm I don't think this makes sense here?
what's the argument for binding the coinbase to the pool?
There was a problem hiding this comment.
I'd expect this to be part of the EthApi.
There was a problem hiding this comment.
I understand that the coinbase would be the beneficiary or fee recipient of minted blocks, so it would be needed for execution payload building. Maybe we could make it part of EthApi, and pass the value to the block builder as a parameter when needed?
| /// Handler for: `eth_coinbase` | ||
| async fn author(&self) -> Result<Address> { | ||
| Err(internal_rpc_err("unimplemented")) | ||
| Ok(self.pool().status().coinbase) |
There was a problem hiding this comment.
hmm, I'm actually not sure if this is sound tbh, because tying coinbase to the pool does not feel right.
unsure when we'd actually need this, I guess for block building but isn't this now part of CL @rakita ?
the coinbase endpoint is also disabled on providers like infura etc.
There was a problem hiding this comment.
Coinbase makes sense to be part of block builder but that is still not defined. Execution is building the block but CL is sending the needed parameters that are used to build the block.
Coinbase is send over EngineAPI from CL part. It is part of https://github.com/ethereum/execution-apis/blob/main/src/engine/shanghai.md#payloadattributesv2 :
suggestedFeeRecipient: DATA, 20 Bytes - suggested value for the feeRecipient field of the new payload
So it can be changed depending on given values from CL. Infur and providers probably disabled it because they are not mining, it was set before merge only if the node is configured as a miner (IIRC).
There was a problem hiding this comment.
Then, should we close this until block building is defined? Or add it as an attribute of EthApi for now, updating it on every payload received from CL?
mattsse
added
A-rpc
4 participants
Ref: #1225
This PR adds the implementation for the
eth_coinbaseendpoint.The coinbase is stored as part of the
Poolconfiguration, and accessed via theTransactionPool::statusmethod.