Skip to content

Conversation

marvinkruse
Copy link
Member

This PR resolves...

  • the TransactionForwarder not being upgradeable (was deployed as upgradeable though, hence the change)
  • the Scripts not catching that, so I added an extensive list of checks that are performed at the end of the deployment
    • Check is Governor owns everything + the right multisigs own the Governor
    • Factories, Governor + Fee Manager are initialized correctly
    • Modules are all deployed with matching metadata
    • Beacons are all pointing to the right reverted and reference the right version
    • TransactionForwarder is initialized correctly (EIP712 data)
  • Typo in the title of a few contracts

@marvinkruse marvinkruse self-assigned this Aug 16, 2024
@marvinkruse marvinkruse changed the title Fix: Resolve Dployment Issues Fix: Resolve Deployment Issues Aug 16, 2024
@Zitzak
Copy link
Collaborator

Zitzak commented Aug 19, 2024

Question around the TxForwarder, I thought it wasn't supposed to be upgradeable, but was deploy as if. But looking at the changes we are going with it being upgradeable? @marvinkruse

Or did I have the wrong?

Copy link
Collaborator

@Zitzak Zitzak left a comment

Choose a reason for hiding this comment

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

LGTM

@marvinkruse
Copy link
Member Author

Question around the TxForwarder, I thought it wasn't supposed to be upgradeable, but was deploy as if. But looking at the changes we are going with it being upgradeable? @marvinkruse

Or did I have the wrong?

No, the discussion was a bit confusing, you don't have anything wrong. Original idea was to have it non-upgradeable, as that is just a bit safer for the user. But that has the big downside that if there is a bug in there, we can't ever change it, as the tx forwarder is hardcoded in each contract. so we'll have it also upgradeable, and with that being behind our timelock etc. it's fine.

That was also the only option where we basically don't change the code (besides the imports), the other option would have been to change all other contracts and have the tx forwarder address be changeable.

Does that make sense?

@marvinkruse marvinkruse merged commit 2a8a4c8 into dev Aug 19, 2024
@marvinkruse marvinkruse deleted the fix-deployment-issues branch August 19, 2024 11:37
0xNuggan pushed a commit that referenced this pull request Mar 24, 2025
* Fix: Turn TransactionForwarder into upgradeable version

* Fix: Remove Reverter from OrchestratorFactory constructor

* Script: Extend verifyDeployment function

* Fix: Remove typo in contract titles
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