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

OptimismPortal set initial balance through StorageSetter pattern #250

Merged
merged 12 commits into from
Oct 8, 2024

Conversation

pahor167
Copy link

@pahor167 pahor167 commented Oct 6, 2024

#193

Reverts a change to OptimismPortal initialize function originally introduced so we can set initialBalance of OptimismPortal. Instead we now use storage setter in following pattern.

  1. Deploy OptimismPortalProxy
  2. Deploy StorageSetter
  3. Set Implementation of OptimismPortalProxy to StorageSetter
  4. Set correct storage slot to initial balance
  5. Set implementation of OptimismPortalProxy to real implementation of OptimismPortal

@pahor167 pahor167 changed the title OptimismPortal revert OptimismPortal set initail balance through StorageSetter pattern Oct 6, 2024
@pahor167 pahor167 changed the title OptimismPortal set initail balance through StorageSetter pattern OptimismPortal set initial balance through StorageSetter pattern Oct 6, 2024
Copy link

@ezdac ezdac left a comment

Choose a reason for hiding this comment

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

Looks good to me!
I added some minor questions / suggestions.

It would be good to implement the storage modification for the OptimismPortal2 as well,
so we include fault-proof compatible contracts already before the code-freeze (see #239), but we can also create a follow-up PR.

The OP style-guide mentions to do all the upgrade steps atomically (modify-storage, change implementation, call initialize). We don't do this here, but I think for the initial deployment this is not as important as upgrading existing contracts on a live-network - do you agree?

@pahor167
Copy link
Author

pahor167 commented Oct 8, 2024

Looks good to me! I added some minor questions / suggestions.

It would be good to implement the storage modification for the OptimismPortal2 as well, so we include fault-proof compatible contracts already before the code-freeze (see #239), but we can also create a follow-up PR.

The OP style-guide mentions to do all the upgrade steps atomically (modify-storage, change implementation, call initialize). We don't do this here, but I think for the initial deployment this is not as important as upgrading existing contracts on a live-network - do you agree?

Since we are owners during the whole process it shouldn't be a problem at all

@ezdac ezdac merged commit fb72612 into celo9 Oct 8, 2024
52 of 53 checks passed
@ezdac ezdac deleted the pahor/optimismPortalBalanceUpdate branch October 8, 2024 22:25
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.

2 participants