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

Emit missing events on important configuration changes #33

Merged
merged 4 commits into from
Dec 12, 2022

Conversation

k1rill-fedoseev
Copy link
Collaborator

@k1rill-fedoseev k1rill-fedoseev commented Dec 6, 2022

Many state-changing operations do not emit events. Consider emitting events for important state
changes.
These operations include:
ZkBobPool:

  • initialize()
  • setTokenSeller()
  • setOperatorManager()

ZkBobAccounting:

  • _setLimits()
  • _resetDailyLimits()
  • _setUsersTier()

BobVault:

  • setYieldAdmin()
  • setInvestAdmin()

MutableOperatorManager:

  • _setOperator()

ERC20Recovery:

  • setRecoveryAdmin()
  • setRecoveredFundsReceiver()
  • setRecoveryLimitPercent()
  • setRecoveryRequestTimelockPeriod()

Claimable:

  • setClaimingAdmin()

Comments to changes:
Event describing the corresponding changes were added to:

  • ZkBobPool: setTokenSeller(), setOperatorManager(), withdrawFee()
  • ZkBobAccounting: _setLimits()
  • MutableOperatorManager: _setOperator()

. For other parameters, they are not expected to be changed regularly and there is no any interest in their retrospective observations, thus such events considered as redundant:

  • ZkBobPool: initialize() – an event will be emitted as part of _setLimits execution
  • ZkBobAccounting: _resetDailyLimits() is not expected to be called regularly; _setUsersTier() already contains the event UpdateTier
  • BobVault: setYieldAdmin() and setInvestAdmin() are not expected to be called regularly
  • ERC20Recovery: setRecoveryAdmin()is not expected to be called regularly;setRecoveredFundsReceiver(), setRecoveryLimitPercent(), setRecoveryRequestTimelockPeriod()` do not require their retrospective observations whereas the current value can be extracted with getters.
  • Claimable: setClaimingAdmin() is not expected to be called regularly

@k1rill-fedoseev k1rill-fedoseev self-assigned this Dec 6, 2022
@k1rill-fedoseev k1rill-fedoseev changed the title Emit event on limits change Emit missing events on important configuration changes Dec 12, 2022
@akolotov akolotov merged commit c774fd2 into master Dec 12, 2022
@akolotov akolotov deleted the audit/dec22/5.6 branch December 12, 2022 13:07
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