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

Implementation and tests #1

Merged
merged 23 commits into from
Feb 10, 2025
Merged

Implementation and tests #1

merged 23 commits into from
Feb 10, 2025

Conversation

0xp3th1um
Copy link
Contributor

No description provided.

amusingaxl
amusingaxl suggested changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@amusingaxl amusingaxl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left a few suggestions.

Also we're going to use Solidity 0.8.24, but both foundry.toml and the source code point to 0.8.26.

src/DssBlow2.sol Outdated Show resolved Hide resolved
src/DssBlow2.sol Outdated Show resolved Hide resolved
src/DssBlow2.t.sol Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
src/DssBlow2.sol Outdated Show resolved Hide resolved
@oddaf oddaf self-requested a review February 4, 2025 20:43
oddaf
oddaf previously approved these changes Feb 5, 2025
View reviewed changes
amusingaxl
amusingaxl suggested changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

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

While this won't be a problem with this repo specifically, it's probably a good idea to keep it consistent with other modules.
We should always have floating pragmas in .sol files, so it's a bit easier to import them in other projects if required.
Then we can fix the compiler version in foundry.toml.

script/DssBlow2Deploy.s.sol Outdated Show resolved Hide resolved
src/DssBlow2.sol Outdated Show resolved Hide resolved
src/DssBlow2.t.sol Outdated Show resolved Hide resolved
src/deployment/DssBlow2Deploy.sol Outdated Show resolved Hide resolved
src/deployment/DssBlow2Instance.sol Outdated Show resolved Hide resolved
script/DssBlow2Deploy.s.sol Outdated Show resolved Hide resolved
amusingaxl
amusingaxl suggested changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

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

Found an additional problem with the interface declarations.

src/DssBlow2.sol Outdated Show resolved Hide resolved
src/DssBlow2.sol Outdated Show resolved Hide resolved
src/DssBlow2.sol Outdated Show resolved Hide resolved
amusingaxl
amusingaxl reviewed Feb 5, 2025
View reviewed changes
src/DssBlow2.sol Outdated Show resolved Hide resolved
amusingaxl
amusingaxl previously approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@amusingaxl amusingaxl requested a review from oddaf February 5, 2025 21:08
amusingaxl
amusingaxl reviewed Feb 7, 2025
View reviewed changes
Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

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

Let's update the license headers (:

script/DssBlow2Deploy.s.sol Outdated Show resolved Hide resolved
src/DssBlow2.sol Outdated Show resolved Hide resolved
src/DssBlow2.t.sol Outdated Show resolved Hide resolved
src/deployment/DssBlow2Deploy.sol Outdated Show resolved Hide resolved
src/deployment/DssBlow2Instance.sol Outdated Show resolved Hide resolved
amusingaxl
amusingaxl reviewed Feb 7, 2025
View reviewed changes
Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

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

Some nits about the license header.

src/DssBlow2.sol Outdated Show resolved Hide resolved
script/DssBlow2Deploy.s.sol Outdated Show resolved Hide resolved
src/deployment/DssBlow2Deploy.sol Outdated Show resolved Hide resolved
src/deployment/DssBlow2Instance.sol Outdated Show resolved Hide resolved
oddaf
oddaf reviewed Feb 7, 2025
View reviewed changes
src/DssBlow2.sol Outdated Show resolved Hide resolved
oddaf
oddaf previously approved these changes Feb 10, 2025
View reviewed changes
amusingaxl
amusingaxl previously approved these changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

oddaf
oddaf previously approved these changes Feb 10, 2025
View reviewed changes
@0xp3th1um 0xp3th1um dismissed stale reviews from oddaf and amusingaxl via 8cffeb8 February 10, 2025 17:19
@0xp3th1um 0xp3th1um merged commit d4e65ed into master Feb 10, 2025
2 checks passed
@0xp3th1um 0xp3th1um deleted the initial-implemenation branch February 10, 2025 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oddaf oddaf oddaf approved these changes

@amusingaxl amusingaxl amusingaxl approved these changes

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
@0xp3th1um @oddaf @amusingaxl