feat: Enable pausing deposits and fills unilaterally #212

nicholaspai · 2022-12-20T01:19:23Z

Useful for upgrades

pxrl · 2022-12-20T21:57:27Z

This generally looks good to me.

I can't think of any reason why we'd want to disable only fills on a specific chain, so I'm tempted to propose that we have "pauseDeposits" and "pause" (all). The reason for this is that in the event that we had some kind of emergency, it'd be a little bit painful to do 2x transactions per Spoke Pool.

An alternative could be to just implement a separate method to pause both deposits and fills in a single txn. That's probably safer. wdyt?

mrice32

LGTM!

How do you think we should think about merging PRs like this, where we aren't sure if they're going to get into the next audit? Should we avoid merging until we are sure this is the code that will be deployed in the upgrade?

nicholaspai · 2022-12-21T17:25:34Z

LGTM!

How do you think we should think about merging PRs like this, where we aren't sure if they're going to get into the next audit? Should we avoid merging until we are sure this is the code that will be deployed in the upgrade?

Hm, why wouldn't this get into next audit?

mrice32 · 2022-12-21T23:40:17Z

LGTM!
How do you think we should think about merging PRs like this, where we aren't sure if they're going to get into the next audit? Should we avoid merging until we are sure this is the code that will be deployed in the upgrade?

Hm, why wouldn't this get into next audit?

@nicholaspai just prioritization! I think it's still unclear exactly what the prioritization will look like. Do you think we should definitely prioritize this feature over the others we've discussed for the audit?

mrice32

#212 (comment)

nicholaspai · 2022-12-22T14:38:03Z

LGTM!
How do you think we should think about merging PRs like this, where we aren't sure if they're going to get into the next audit? Should we avoid merging until we are sure this is the code that will be deployed in the upgrade?

Hm, why wouldn't this get into next audit?

@nicholaspai just prioritization! I think it's still unclear exactly what the prioritization will look like. Do you think we should definitely prioritize this feature over the others we've discussed for the audit?

I think the SpokePool upgrades are the highest priority and this would be a subset of that feature, so I do think its worth adding?

pxrl · 2022-12-27T13:57:41Z

contracts/SpokePool.sol

+
+    function pauseFills(bool pause) public override onlyAdmin nonReentrant {
+        pausedFills = pause;
+        emit PausedFills(pausedDeposits);


The event here looks incorrect - should emit the value of pausedFills instead of pausedDeposits.

great catch

pxrl · 2022-12-27T14:08:27Z

contracts/SpokePool.sol

@@ -317,7 +348,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
        uint64 newRelayerFeePct,
        uint32 depositId,
        bytes memory depositorSignature
-    ) public override nonReentrant {
+    ) public override nonReentrant unpausedDeposits {


Is it desirable to make speedups subject to deposits not being paused?

As I see it, the point of pausing deposits is to stop additional funds from entering the Spoke Pool. In the case of a speedup, the funds are already there, and are presumably stuck because the deposit was never filled on the destination chain.

Additionally, as far as I can see, the only side-effect of a speedup is that the Spoke Pool emits another event. It's up to the bots as to whether they accept a speedup event, and it will implicitly be conditional on fills being enabled on the destination chain anyway.

Based on the above, I feel like we should not apply this modifier to speedUpDeposit(). wdyt?

hmm I think its a bit easier to reason about from the bot's perspective of all deposit related events stop once a contract is paused, even though its philosophically ok to not pause speed ups. I was thinking pausing deposits and fills is more of a bot convenience so the owner of the contracts can effectively downgrade a contract by stopping all of its events

Fair enough - I can definitely see the argument for keeping it simple.

There is a theoretical possibility for the edge case I described, and an annoying scenario would be that we have a number of underpriced deposits that are suddenly marooned in the Spoke Pool. In this case we'd need to either unpause to allow speedups to be submitted, or just take the hit and fill these relays at a discount. In theory the relayerFeePct could be very very low.

It's a theoretical possibility, and probably unlikely to be a reality. As long as we're OK with the possibility of it arising then I have no additional input on this one :)

hmmm ok that's a valid concern, i don't love the idea that someone will try to use the speedup signatures to replay attack but I guess its not really an attack since they could have chosen to call speedUp on either the old or the new spoke pool at the same cost with no extra benefit

test/SpokePool.Admin.ts

into npai/pause

pxrl · 2022-12-27T15:12:09Z

contracts/SpokePool.sol

+    event PausedDeposits(bool indexed isPaused);
+    event PausedFills(bool indexed isPaused);


Minor: Do these need to be indexed? I imagine that there will be so few of them that there's not much advantage to filtering on the "paused" state.

mrice32

LGTM once @pxrl's comments are addressed!

mrice32 · 2022-12-29T06:44:29Z

LGTM!
How do you think we should think about merging PRs like this, where we aren't sure if they're going to get into the next audit? Should we avoid merging until we are sure this is the code that will be deployed in the upgrade?

Hm, why wouldn't this get into next audit?

@nicholaspai just prioritization! I think it's still unclear exactly what the prioritization will look like. Do you think we should definitely prioritize this feature over the others we've discussed for the audit?

I think the SpokePool upgrades are the highest priority and this would be a subset of that feature, so I do think its worth adding?

Tbh, I don't think it's a big deal to go ahead and merge even if we later decide to drop this feature, as it would just be a simple git revert. We have an upcoming meeting to figure this out, but regardless, LGTM, as that shouldn't hold this PR up IMO.

pxrl

LGTM!

feat: Enable pausing deposits and fills unilaterally

ec030d0

Useful for upgrades

nicholaspai requested review from mrice32 and pxrl December 20, 2022 01:19

mrice32 approved these changes Dec 21, 2022

View reviewed changes

nicholaspai requested a review from mrice32 December 21, 2022 17:35

mrice32 reviewed Dec 22, 2022

View reviewed changes

nicholaspai requested a review from mrice32 December 22, 2022 14:38

pxrl reviewed Dec 27, 2022

View reviewed changes

test/SpokePool.Admin.ts Outdated Show resolved Hide resolved

nicholaspai added 2 commits December 27, 2022 09:23

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

56f93e7

into npai/pause

Fix tests

6a13915

pxrl reviewed Dec 27, 2022

View reviewed changes

mrice32 approved these changes Dec 29, 2022

View reviewed changes

pxrl mentioned this pull request Dec 29, 2022

feat: Add upgradeability features to SpokePool #208

Closed

Update SpokePool.sol

290d3ec

nicholaspai requested a review from pxrl December 30, 2022 00:51

pxrl approved these changes Dec 30, 2022

View reviewed changes

nicholaspai merged commit f0f1a03 into master Dec 30, 2022

nicholaspai deleted the npai/pause branch December 30, 2022 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable pausing deposits and fills unilaterally #212

feat: Enable pausing deposits and fills unilaterally #212

nicholaspai commented Dec 20, 2022

pxrl commented Dec 20, 2022

mrice32 left a comment

nicholaspai commented Dec 21, 2022

mrice32 commented Dec 21, 2022 •

edited

Loading

mrice32 left a comment

nicholaspai commented Dec 22, 2022

pxrl Dec 27, 2022

nicholaspai Dec 27, 2022

pxrl Dec 27, 2022

nicholaspai Dec 27, 2022

pxrl Dec 27, 2022

nicholaspai Dec 30, 2022 •

edited

Loading

pxrl Dec 27, 2022

mrice32 Dec 29, 2022

mrice32 left a comment

mrice32 commented Dec 29, 2022 •

edited

Loading

pxrl left a comment

		event PausedDeposits(bool indexed isPaused);
		event PausedFills(bool indexed isPaused);

feat: Enable pausing deposits and fills unilaterally #212

feat: Enable pausing deposits and fills unilaterally #212

Conversation

nicholaspai commented Dec 20, 2022

pxrl commented Dec 20, 2022

mrice32 left a comment

Choose a reason for hiding this comment

nicholaspai commented Dec 21, 2022

mrice32 commented Dec 21, 2022 • edited Loading

mrice32 left a comment

Choose a reason for hiding this comment

nicholaspai commented Dec 22, 2022

pxrl Dec 27, 2022

Choose a reason for hiding this comment

nicholaspai Dec 27, 2022

Choose a reason for hiding this comment

pxrl Dec 27, 2022

Choose a reason for hiding this comment

nicholaspai Dec 27, 2022

Choose a reason for hiding this comment

pxrl Dec 27, 2022

Choose a reason for hiding this comment

nicholaspai Dec 30, 2022 • edited Loading

Choose a reason for hiding this comment

pxrl Dec 27, 2022

Choose a reason for hiding this comment

mrice32 Dec 29, 2022

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

mrice32 commented Dec 29, 2022 • edited Loading

pxrl left a comment

Choose a reason for hiding this comment

mrice32 commented Dec 21, 2022 •

edited

Loading

nicholaspai Dec 30, 2022 •

edited

Loading

mrice32 commented Dec 29, 2022 •

edited

Loading