-
Notifications
You must be signed in to change notification settings - Fork 242
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
chore(core/types): remove Blocks
's Timestamp()
method
#1429
base: master
Are you sure you want to change the base?
Changes from all commits
b1401af
4b34cac
f93ccd3
2c8ef06
6b72fc5
138f940
999319a
a2e09b5
5366533
1cef103
a9b334c
d590a04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,8 +21,8 @@ type AllowListConfig struct { | |
} | ||
|
||
// Configure initializes the address space of [precompileAddr] by initializing the role of each of | ||
// the addresses in [AllowListAdmins]. | ||
func (c *AllowListConfig) Configure(chainConfig precompileconfig.ChainConfig, precompileAddr common.Address, state contract.StateDB, blockContext contract.ConfigurationBlockContext) error { | ||
// the addresses in [AllowListConfig]. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we keep the arguments for this method for symmetry with the other methods (even though unused)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand what symmetry you are thinking about? With the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was my thought, even though the AllowList is not a precompile it follows through the phases of Configure along with the precompile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related question: what would be the cost of changing the signature of AllowListConfig.Configure if needed in the future? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Less than the other method, as the AllowList is not part of the precompile interface but more a partial implementation that's used by some of the precompiles. As I mentioned I have a subjective preference for methods that do similar things to all take the same arguments but this is not a strong preference. |
||
func (c *AllowListConfig) Configure(chainConfig precompileconfig.ChainConfig, precompileAddr common.Address, state contract.StateDB) error { | ||
for _, enabledAddr := range c.EnabledAddresses { | ||
SetAllowListRole(state, precompileAddr, enabledAddr, EnabledRole) | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
this is a breaking change to all precompiles (external and internal precompiles). While having a simple params is better, we should also evaluate this with the future extension possibilities. IMO having a blockContext interface is less cumbersome to extend.