Skip to content

Conversation

@fusmanii
Copy link
Contributor

@fusmanii fusmanii commented Oct 7, 2025

It was missing as part of #1087

@fusmanii fusmanii marked this pull request as ready for review October 7, 2025 18:50
@fusmanii fusmanii requested a review from grasphoper October 7, 2025 18:50
nicholaspai
nicholaspai previously approved these changes Oct 7, 2025
Copy link
Contributor

@pxrl pxrl left a comment

Choose a reason for hiding this comment

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

LGTM - one question on proxy deployment.

"OP_SpokePool",
constructorArgs,
initArgs,
true // implementationOnly
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to read this from the environment, or try to infer it from existing config. If they find a proxy then they skip deploying a new one, and otherwise they deploy both implementation and proxy.

Copy link
Contributor Author

@fusmanii fusmanii Oct 8, 2025

Choose a reason for hiding this comment

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

that is already being done here upstream, this flag is just for when we want to forcefully deploy the implementation only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

set the flag to false 0625b2f

Comment on lines 12 to 15
// 2. forge script script/039DeployOPSpokePool.s.sol:DeployOPSpokePool --rpc-url $NODE_URL_1 -vvvv
// 3. Verify the above works in simulation mode.
// 4. Deploy with:
// forge script script/039DeployOPSpokePool.s.sol:DeployOPSpokePool --rpc-url \
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor:

Suggested change
// 2. forge script script/039DeployOPSpokePool.s.sol:DeployOPSpokePool --rpc-url $NODE_URL_1 -vvvv
// 3. Verify the above works in simulation mode.
// 4. Deploy with:
// forge script script/039DeployOPSpokePool.s.sol:DeployOPSpokePool --rpc-url \
// 2. forge script script/025DeployOPSpokePool.s.sol:DeployOPSpokePool --rpc-url $NODE_URL_1 -vvvv
// 3. Verify the above works in simulation mode.
// 4. Deploy with:
// forge script script/025DeployOPSpokePool.s.sol:DeployOPSpokePool --rpc-url \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 0625b2f

Signed-off-by: Faisal Usmani <[email protected]>
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