Upgrade Solidity to 0.8.20 and Fix Tests#29
Open
benzntech wants to merge 7 commits intobitbankinc:masterfrom
Open
Upgrade Solidity to 0.8.20 and Fix Tests#29benzntech wants to merge 7 commits intobitbankinc:masterfrom
benzntech wants to merge 7 commits intobitbankinc:masterfrom
Conversation
junderw
reviewed
Apr 11, 2025
junderw
reviewed
Apr 11, 2025
| address... since we can't stop ERC20 deposits, we want to leave a path for | ||
| fund recovery if the user deposits to an old deposit address after it's been | ||
| killed. | ||
| - **Solidity Version:** ^0.8.20 |
junderw
reviewed
Apr 11, 2025
| - Fixed `address` to `address payable` explicit conversion errors introduced in Solidity 0.8+. | ||
| - Updated test file (`test/exchangedeposit.js`) assertions (`assert.rejects`) to match new revert reason string formats. | ||
| - Adjusted gas cost expectations in tests. | ||
| - **Note:** Due to older dependencies in the project structure, `npm install --legacy-peer-deps` is currently required to resolve peer dependency conflicts. |
Member
There was a problem hiding this comment.
The npm ci restriction was originally added because we needed to use a special version of prettier.
I would appreciate it if you could fix the dependencies so that we don't need special install flags.
junderw
requested changes
Apr 11, 2025
Member
junderw
left a comment
junderw
left a comment
There was a problem hiding this comment.
Thanks for the PR. Would appreciate some changes I made comments about.
Member
|
I got the CI to run the tests and some are failing. If you could please remedy this along with your other changes I would appreciate it. |
junderw
reviewed
Jun 11, 2025
| const COLD_ADDRESS2 = accounts[7]; | ||
| const ADMIN_ADDRESS2 = accounts[8]; | ||
| const FUNDER_ADDRESS = accounts[9]; | ||
| const FUNDER_ADDRESS = accounts[9]; // This is accounts[9] |
junderw
requested changes
Jun 11, 2025
Member
junderw
left a comment
junderw
left a comment
There was a problem hiding this comment.
Thanks for the work!
Tests are failing. And 2 of the issues mentioned before were left unresolved.
Also I think there was an accidental comment left (see the "?")
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR upgrades the project's smart contracts from Solidity version 0.6.11 to ^0.8.20.
Changes Included:
pragmastatements in all contracts.hardhat.config.js.@openzeppelin/contractsdependency to v4.9.3 (compatible with Solidity 0.8).prettierdependency to resolve peer conflicts.address payableconversions and updated OpenZeppelin import paths.test/exchangedeposit.js) to useassert.rejectswith adjusted regular expressions matching the new revert reason formats from Solidity 0.8.All tests in the existing suite (34 tests) are passing after these changes.
Note: Due to the older dependency structure in the
package.json, runningnpm install --legacy-peer-depsis currently required to successfully install dependencies.