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

Upgradable contracts implementation #43

Merged
merged 17 commits into from
Aug 22, 2024

Conversation

ccorcoveanu
Copy link
Collaborator

No description provided.

dragos-rebegea
dragos-rebegea previously approved these changes Aug 21, 2024
Copy link
Contributor

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

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

You should have pushed also the .hex, .go and .abi.json compiled contracts

@@ -52,6 +53,18 @@ contract ERC20Safe is BridgeRole, Pausable {
event ERC20Deposit(uint112 depositNonce, uint112 batchId);
event ERC20SCDeposit(uint112 indexed batchId, uint112 depositNonce, string callData);
Copy link
Contributor

Choose a reason for hiding this comment

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

if you compile the sol contracts, this event will have the CallData field defined as string not []byte. @dragos-rebegea did a recent change in which we changed this field from string to []byte to avoid double hex encoding. How do we fix this?

Copy link
Collaborator Author

@ccorcoveanu ccorcoveanu Aug 22, 2024

Choose a reason for hiding this comment

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

merged Dragos's changes, should be good now

Copy link
Contributor

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

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

changed the bridge contract to have an empty constructor (0 args) and manually called the initialize function after deployment.
When I try to call the function SetBridge afterwards, it errors with execution reverted: Access Control: sender is not Admin

@ccorcoveanu
Copy link
Collaborator Author

You should have pushed also the .hex, .go and .abi.json compiled contracts

@iulianpascalau I will push them now, but I don't really like this approach, even small PRs become very bloated with generated files, maybe we can think of an alternative?

@iulianpascalau
Copy link
Contributor

You should have pushed also the .hex, .go and .abi.json compiled contracts

@iulianpascalau I will push them now, but I don't really like this approach, even small PRs become very bloated with generated files, maybe we can think of an alternative?

Yes, we can think of a better system for generating binaries. Maybe a docker image that will do all the time this thing for us and publish the binaries as artifacts at release time? 🤔

@dragos-rebegea dragos-rebegea merged commit 17c69fd into feat/upgradable-contracts Aug 22, 2024
1 check passed
@dragos-rebegea dragos-rebegea deleted the upgradable-contracts-impl branch August 22, 2024 11:04
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