search

thehubbleproject / hubble-contracts

Hubble optimistic rollup
https://thehubbleproject.github.io/docs/
MIT License
133 stars 28 forks source link

Inconsistent deposit subtree IDs after rollback #672

Closed b-tarczynski closed 2 years ago

b-tarczynski commented 2 years ago

Resolves https://github.com/thehubbleproject/hubble-contracts/issues/671

Decided to follow naming proposed by @jacque006 and not to emit duplicated DepositSubTreeReady event on reenqueue function. Let me know if you find any places for improvement.

b-tarczynski commented 2 years ago

Contract changes look good to me. @ChihChengLiang if you have time your review of the DepositManager.sol changes would be appreciated.

Although I see some benefits of the thoroughness of the new slow deposit test, it will increase the overall time the full test suite will take to run. Was there a reason you opted for the new slow test instead of modifying the existing Test rollback with deposits test case?

I decided to create slow test for deposits, because existing Test rollback with deposits was using MockDepositManager which reenqueue function only emits DepositSubTreeReady and production DepositManager doesn't do that anymore. So if I had left emit in mock implementation, test would have checked emitted deposits in invalid order. Right now MockDepositManager reenqueue function does nothing.