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

add delayed upgrade #2735

Open
wants to merge 1 commit into
base: release_loopring_3.6.3
Choose a base branch
from

Conversation

kl456123
Copy link
Contributor

No description provided.

// Enable the circuit
circuit.enabled = true;

emit CircuitDisabled(blockType, blockSize, blockVersion);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is enabled ,can add an event

nextEffectiveTime = _nextEffectiveTime;
}

emit SettingsUpdated(block.timestamp, _nextEffectiveTime);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not modify current event, can add another event which only update but not effect

@@ -55,6 +55,16 @@ abstract contract IBlockVerifier is Claimable
external
virtual;

/// @dev Enables the use of the specified circuit.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// @dev Enables the use of the specified circuit.
/// @dev Enables the use of a specified circuit.

mapping (address => uint) internal exchangeStake;

uint public nextEffectiveTime;
Copy link

@dantaik dantaik Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change the data storage layout table and break upgradeability!!!!!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you have contract LoopringV3 is ILoopringV3, ReentrancyGuard, changing any storage layout in IloopringV3 will break upgradabilty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loopringv3 is not deployed using proxy mode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it has no initialize function

@@ -17,11 +17,13 @@ contract BlockVerifier is ReentrancyGuard, IBlockVerifier
struct Circuit
{
bool registered;
uint registeredTime;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will you deploy a new BlockVerifier or upgrade an existing deployment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blockverifier is the same case

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you are modifying the code, so what did you mean "is the same"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for my confused statement. I mean that block verifier also cannot be deployed by using proxy mode like in loopringv3 so that only way to upgrade smart contract is to deploy a new one. so the disorder of storage of it doesn't matter.

@dantaik
Copy link

dantaik commented Sep 19, 2024

@letsgoustc @kl456123 I think "delayed upgrade" is a general feature to all contracts. It's supposed to be contract-agnostic, you probably just need a wrapping "owner contract" that implements a delay internally without any change to the contracts being owned.

@dantaik
Copy link

dantaik commented Sep 19, 2024

I think we should schedule a call for discussion.

@kl456123
Copy link
Contributor Author

@letsgoustc @kl456123 I think "delayed upgrade" is a general feature to all contracts. It's supposed to be contract-agnostic, you probably just need a wrapping "owner contract" that implements a delay internally without any change to the contracts being owned.

Yes I also think it is absolutely feasible and might be a better way to reach the same goal

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.

3 participants