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

Spell clean up #210

Merged
merged 25 commits into from
Jan 3, 2022
Merged

Spell clean up #210

merged 25 commits into from
Jan 3, 2022

Conversation

iamchrissmith
Copy link
Contributor

@iamchrissmith iamchrissmith commented Dec 14, 2021

Clean up tasks for the DSS Spell.

It will likely be easier to review in parts. I've mostly batched changes into specific commits except collateral onboarding which is covered in the last few commits (21eb5a9 - f57951e)

List of tasks: https://github.com/makerdao/smart-contracts-team-pm/issues/747

@iamchrissmith iamchrissmith marked this pull request as ready for review December 15, 2021 00:11
src/DssSpell.t.base.sol Outdated Show resolved Hide resolved
src/DssSpell.t.base.sol Outdated Show resolved Hide resolved
src/DssSpell.t.base.sol Outdated Show resolved Hide resolved
test-dev-dssspell.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
src/DssSpell.t.base.sol Outdated Show resolved Hide resolved
src/test/collaterals.sol Outdated Show resolved Hide resolved
src/DssSpell.t.base.sol Outdated Show resolved Hide resolved
src/DssSpell.t.base.sol Outdated Show resolved Hide resolved
src/test/collaterals.sol Outdated Show resolved Hide resolved
src/DssSpell.t.base.sol Outdated Show resolved Hide resolved
src/DssSpell.t.base.sol Outdated Show resolved Hide resolved
naszam
naszam previously approved these changes Dec 17, 2021
Copy link
Contributor

@naszam naszam left a comment

Choose a reason for hiding this comment

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

LGTM

naszam
naszam previously approved these changes Dec 17, 2021
Makefile Outdated
clean :; dapp clean
# Usage example: make test match=SpellIsCast
test :; ./test-dssspell.sh $(match)
test-dev :; ./test-dev-dssspell.sh $(match)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than having two separate scripts here, what about one script that accepts an argument to enable dev mode?

That way when we make other changes as a result of tooling updates or whatever we don't need to reflect them in two separate files. In this case, the only real difference will be whether the optimizer is off or on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated: fdc1a0b

I left the test-dev command in there for shorthand, let me know if that bothers you

test-dev-dssspell.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@brianmcmichael brianmcmichael left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@naszam naszam left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@iamchrissmith iamchrissmith merged commit 01d06d3 into master Jan 3, 2022
@iamchrissmith iamchrissmith deleted the spell-clean-up branch January 3, 2022 22:01
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