-
Notifications
You must be signed in to change notification settings - Fork 30
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
Modularize zkBob pool and add energy redeemer example #78
Conversation
c731517
to
3d1f1b3
Compare
* @param _maxWeeklyAvgTvl max seen average tvl over period of at least 1 week, might not be precise. | ||
*/ | ||
function initialize( | ||
uint32 _txCount, |
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.
Does it make sense to copy this state from the pool contract during initialisation rather than specify them manually?
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.
I didn't want to include logic for raw storage reads in the main contract, so instead I placed it in the one-time migration contract. I think it's a bit safer and flexible approach overall.
In case we will ever again need to redeploy accounting module (e.g. to change limits logic), such generic initialization logic would be also helpful.
@@ -329,7 +324,10 @@ abstract contract ZkBobPool is IZkBobPool, EIP1967Admin, Ownable, Parameters, Zk | |||
*/ | |||
function recordDirectDeposit(address _sender, uint256 _amount) external { |
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.
Does it make sense to call recordOperation
from the queue contract directly? What is the goal of having this functionality in the pool contract?
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.
I think it's better to keep it this way, to reduce dependencies between pool modules. It potentially simplifies future upgrades, as there less address pointers being used between various contracts and permission logic in contract becomes simpler.
20623dc
to
39254e9
Compare
39254e9
to
0d46f96
Compare
This PR simplifies logic of core ZkBobPool contract, by extracting limits, accounting, and KYC logic into an external contract.
Changelog
Changes in the ZkBobPool interface:
zkBobPool.getLimitsFor(...)
->zkBobPool.accounting().getLimitsFor(...)
zkBobPool.kycProvidersManager()
->zkBobPool.accounting().kycProvidersManager()
Changes in denomination:
accounting.setLimits(...)
now expects values in zkBob units, without denominatorMigration details
There are a few data structures that are being deprecated during the migration:
slot0
andslot1
. These should be copied to the new module during the migration.maxWeeklyAvgTvl
andmaxWeeklyTxCount
stats.