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

Feat: add PSM checklists #34

Merged
merged 9 commits into from
Jul 22, 2024
Merged

Feat: add PSM checklists #34

merged 9 commits into from
Jul 22, 2024

Conversation

amusingaxl
Copy link
Contributor

No description provided.

@amusingaxl amusingaxl self-assigned this Jul 18, 2024
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
spell/psm-checklists.md Show resolved Hide resolved
spell/psm-checklists.md Outdated Show resolved Hide resolved
- [ ] `DssLitePsmMom` is added to the chainlog and `addresses_mainnet.sol` according to the Exec Sheet
- [ ] `LitePsmJob` is added to the chainlog and `addresses_mainnet.sol` according to the Exec Sheet
- [ ] IF a new `pip` is being added, it is added to the chainlog and `addresses_mainnet.sol` as `PIP_{TOKEN_SYMBOL}` (i.e. `PIP_USDC`)
- Test coverage:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please add end-to-end test here to check main functionally of the DssLitePsm?

As far as I can see, PSMs are not tested by any of the base tests, and testPSMs have to be specifically extended to call _checkPsmIlkIntegration for this each PSM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.
_checkPsmIlkIntegration is not LitePSM friendly, it assumes there will be clip and a gemJoin for all PSMs.
Maybe a simple test to buy and sell gems with it would suffice. Not sure if it would belong in the base tests though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we expect to deploy more LitePSM modules in the future, I think it makes sense to add a base test to do most of the checks described in the Test coverage. Having a checklist is good, but having a base/reusable/configurable test is even better in my opinion.

@amusingaxl amusingaxl merged commit e2376d4 into master Jul 22, 2024
2 checks passed
@amusingaxl amusingaxl deleted the feat/psm-checklists branch July 22, 2024 17:37
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