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

fix wrong domain in deploy script #1376

Merged
merged 3 commits into from
Sep 14, 2023
Merged

fix wrong domain in deploy script #1376

merged 3 commits into from
Sep 14, 2023

Conversation

trajan0x
Copy link
Contributor

@trajan0x trajan0x commented Sep 14, 2023

Description

Uncovered while working on #1274, I appear to have broken this in #1308. The agent manager constructor was still passed the local domain instead of the synapse domain

Summary by CodeRabbit


  • Refactor: Updated the domain variable name from localDomain to synapseDomain across multiple scripts in the contracts-core package. This change ensures consistency and clarity in the codebase.
  • Bug Fix: Corrected the constructor arguments for deploying various contracts such as LightManager, LightInbox, BondingManager, and Inbox. The updated domain address (synapseDomain) is now correctly passed during these deployments, ensuring proper contract initialization.

@github-actions github-actions bot added M-contracts Module: Contracts 2-reviewers C-Protocol-Critical PRs that modify protocol-critical code. size/xs labels Sep 14, 2023
@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.40696% ⚠️

Comparison is base (c9df345) 50.77047% compared to head (0f395bc) 50.36351%.

Additional details and impacted files
@@                 Coverage Diff                 @@
##              master       #1376         +/-   ##
===================================================
- Coverage   50.77047%   50.36351%   -0.40697%     
===================================================
  Files            356         346         -10     
  Lines          24920       24071        -849     
  Branches         277         277                 
===================================================
- Hits           12652       12123        -529     
+ Misses         10982       10723        -259     
+ Partials        1286        1225         -61     
Flag Coverage Δ
cctp-relayer ?
packages 83.42541% <ø> (ø)
promexporter ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@trajan0x trajan0x marked this pull request as ready for review September 14, 2023 23:04
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 14, 2023

Walkthrough

The changes primarily involve renaming the localDomain variable to synapseDomain across multiple scripts. This modification affects the arguments passed to various deployment functions, ensuring contracts are deployed with the correct domain address.

Changes

File Path Summary
.../script/DeployMessaging003Base.s.sol Renamed localDomain to synapseDomain in constructorArgs. Affects factoryDeploy function arguments.
.../script/DeployMessaging003LightChain.s.sol Replaced localDomain with synapseDomain in _deployAgentManager and _deployStatementInbox functions. Affects LightManager and LightInbox contract deployments.
.../script/DeployMessaging003SynChain.s.sol Substituted localDomain with synapseDomain for deploying BondingManager and Inbox contracts.

🐇💻

In the land of code where the shadows lie,

We rename a variable, no need to sigh.

From local to synapse, the domain does fly,

Ensuring our contracts reach the sky. 🚀🌌


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between c9df345 and 0f395bc commits.
Files selected for processing (3)
  • packages/contracts-core/script/DeployMessaging003Base.s.sol (1 hunks)
  • packages/contracts-core/script/DeployMessaging003LightChain.s.sol (2 hunks)
  • packages/contracts-core/script/DeployMessaging003SynChain.s.sol (2 hunks)
Files skipped from review due to trivial changes (2)
  • packages/contracts-core/script/DeployMessaging003Base.s.sol
  • packages/contracts-core/script/DeployMessaging003SynChain.s.sol
Additional comments (Suppressed): 2
packages/contracts-core/script/DeployMessaging003LightChain.s.sol (2)
  • 21-27: The change from localDomain to synapseDomain seems appropriate given the PR's goal. However, ensure that synapseDomain is correctly initialized and has a valid value before this function is called.

  • 41-47: Similar to the previous comment, the change from localDomain to synapseDomain aligns with the PR's intent. Again, verify that synapseDomain is properly initialized and holds a valid value before this function is invoked.

@trajan0x
Copy link
Contributor Author

This has been tested in #1274 and works (deploys work again).

@trajan0x trajan0x merged commit a7a6bbb into master Sep 14, 2023
@trajan0x trajan0x deleted the domain-fix branch September 14, 2023 23:09
@trajan0x trajan0x mentioned this pull request Sep 15, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2-reviewers C-Protocol-Critical PRs that modify protocol-critical code. M-contracts Module: Contracts size/xs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant