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.

Sign up for GitHub

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

Jump to bottom

Update pragma to most restrictive dependency or feature #1461

Closed
wants to merge 1 commit into from
Closed

Update pragma to most restrictive dependency or feature #1461

wants to merge 1 commit into from

Conversation

RyanRHall
Copy link
Contributor

@RyanRHall RyanRHall commented Sep 23, 2024 •
edited
Loading

Motivation

We have several contracts using ^0.8.0 but also custom errors, or a child contract with custom errors. We also have several contracts at ^0.8.0 but a dependency locked at 0.8.24.

Solution

Update all pragmas to the pragma of the most restrictive dep or the minimum to enable custom errors (0.8.4)

RyanRHall
RyanRHall commented Sep 23, 2024
View reviewed changes
contracts/src/v0.8/ccip/applications/DefensiveExample.sol
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
pragma solidity ^0.8.20;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

EnumerableMap is ^0.8.20

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ohh that's not great. Requiring 0.8.20 feels restrictive. Can we downgrade OZ to 4.8.3 for the examples?

RyanRHall
RyanRHall commented Sep 23, 2024
View reviewed changes
contracts/src/v0.8/ccip/applications/SelfFundedPingPong.sol
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
pragma solidity 0.8.24;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Router is 0.8.24. This could be left open by switching to IRouter (which should be the default approach in the future 🙂)

Copy link
Contributor

Choose a reason for hiding this comment

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

no ^?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has to be 0.8.24 bc Router is fixed at 0.8.24. It would compile if the made the pragma less restrictive but it would only ever be able to use sol 0.8.24 so there's no reason to open the pragma.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is fine to be at 24 as it's only used internally anyway

RyanRHall
RyanRHall commented Sep 23, 2024
View reviewed changes
contracts/src/v0.8/ccip/pools/LegacyPoolWrapper.sol
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.0;
pragma solidity 0.8.24;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TokenPool is 0.8.24

RyanRHall
RyanRHall commented Sep 23, 2024
View reviewed changes
contracts/src/v0.8/ccip/test/helpers/MaybeRevertingBurnMintTokenPool.sol
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
pragma solidity 0.8.24;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BurnMintTokenPool is 0.8.24

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 23, 2024 •
edited
Loading

LCOV of commit 2af21b5 during Solidity Foundry #8331

Summary coverage rate:
  lines......: 97.7% (2281 of 2335 lines)
  functions..: 94.6% (421 of 445 functions)
  branches...: 93.6% (542 of 579 branches)

Files changed coverage rate: n/a

@RyanRHall RyanRHall force-pushed the CCIP-3121-multi-on-ramp-emit-sequence-number-with-ccip-send-requested-event branch from 3ee66fe to 2af21b5 Compare September 23, 2024 20:52
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

RensR
RensR reviewed Sep 24, 2024
View reviewed changes
contracts/src/v0.8/ccip/test/legacy/BurnMintTokenPool1_2.sol
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;
pragma solidity ^0.8.4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

uses OZ 5 so let's use 0.8.24? It's a test contract anyway. Can you also update the license

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good catch on OZ 5

@RyanRHall
Copy link
Contributor Author

Closing in favor of #1466

@RyanRHall RyanRHall closed this Sep 24, 2024
@RyanRHall RyanRHall deleted the CCIP-3121-multi-on-ramp-emit-sequence-number-with-ccip-send-requested-event branch September 24, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RensR RensR RensR left review comments

@makramkd makramkd Awaiting requested review from makramkd makramkd is a code owner

@elatoskinas elatoskinas Awaiting requested review from elatoskinas elatoskinas is a code owner

@RayXpub RayXpub Awaiting requested review from RayXpub RayXpub is a code owner

@jhweintraub jhweintraub Awaiting requested review from jhweintraub

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RyanRHall @RensR @jhweintraub