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: add system config consensus to deprecate clique #72

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ranchalp
Copy link

In conjunction with go-ethereum#1102.

@ranchalp ranchalp requested a review from zimpha January 15, 2025 13:16
@ranchalp ranchalp force-pushed the system-contract branch 4 times, most recently from ececf8e to e5f797e Compare January 15, 2025 13:24
*/
contract SystemSignerRegistry is Ownable {
struct Signer {
uint64 startBlock;
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we decide against having the startBlock here?

address signer;
}

Signer[] private signers;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Signer[] private signers;
mapping (address => bool) public isAuthorizedSigner;

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to consider how l2geth will read this contract.

  • If it just polls the latest state asking "am I authorized?", then a mapping is suitable.
  • If it just polls the latest state asking "what is the list of current authorized signers?", then an array is suitable.
  • If it fetches events, then I'd still go with mapping.

Copy link
Member

Choose a reason for hiding this comment

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

will we have multiple authorized signer in the same time?

* The getSigners() function returns parallel arrays for block numbers and addresses,
*
*/
contract SystemSignerRegistry is Ownable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to make this upgradable?

* The getSigners() function returns parallel arrays for block numbers and addresses,
*
*/
contract SystemSignerRegistry is Ownable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not model this after OP's? https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/L1/SystemConfig.sol (see for example _setUnsafeBlockSigner).

@zimpha what do you think about storing multiple configs here (including gas coefficients, max gas limit, etc.)? Or is that too expensive gas-wise?

Copy link
Member

Choose a reason for hiding this comment

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

we can add these config here, we can pack related parameters into single uint256. Then the gas overhead will be minimum.

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.

3 participants