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

[SC-105] Rewrite tests & remove 'aave-helpers' dependency #2

Merged
merged 41 commits into from
Sep 25, 2023

Conversation

barrutko
Copy link
Contributor

No description provided.

Copy link
Contributor

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

Two minor issues. This is a good first pass translating the previous version. I'd like to do another PR after where the test logic is merged into a single base test contract and the specifics of the domain are dealt with by inheriting contracts. Similar to how this is done: https://github.com/makerdao/dss-bridge/tree/v1/src/tests/domains

Would also like to see the testing expanded to cover more of the features, but I think ordering can be so as to merge the test contracts first then adding more tests after.

foundry.toml Outdated Show resolved Hide resolved
.gitmodules Outdated Show resolved Hide resolved
@barrutko
Copy link
Contributor Author

The xchain-helpers appears to be referencing a non-existent commit. Not sure what's going on here.

Should be fine now.

Copy link
Contributor

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

Sorry I jumped the gun a bit with my last review. I didn't realize you merged 3 tasks into this PR.

Overall looks good, just some best practises to follow.

test/CrosschainTestBase.sol Outdated Show resolved Hide resolved
test/CrosschainTestBase.sol Outdated Show resolved Hide resolved
test/CrosschainTestBase.sol Outdated Show resolved Hide resolved
test/CrosschainTestBase.sol Outdated Show resolved Hide resolved
test/CrosschainTestBase.sol Outdated Show resolved Hide resolved
test/CrosschainTestBase.sol Outdated Show resolved Hide resolved
test/CrosschainTestBase.sol Outdated Show resolved Hide resolved
test/mocks/ReconfigurationPayload.sol Outdated Show resolved Hide resolved
hexonaut
hexonaut previously approved these changes Sep 20, 2023
Copy link
Contributor

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

LGTM

test/CrosschainTestBase.sol Outdated Show resolved Hide resolved
hexonaut
hexonaut previously approved these changes Sep 21, 2023
test/CrosschainTestBase.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@lucas-manuel lucas-manuel left a comment

Choose a reason for hiding this comment

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

LGTM for a start, will build on this and add more testing as needed

@barrutko barrutko merged commit dce1e4f into master Sep 25, 2023
@barrutko barrutko deleted the SC-105-rewrite-tests branch September 25, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants