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

StorageReadable unit tests #154

Merged
merged 9 commits into from
Jun 5, 2024
Merged

StorageReadable unit tests #154

merged 9 commits into from
Jun 5, 2024

Conversation

kamuik16
Copy link
Contributor

@kamuik16 kamuik16 commented Jun 4, 2024

Closes #146

Foundry unit tests for StorageReadable contract.

@kamuik16 kamuik16 requested a review from a team as a code owner June 4, 2024 14:49
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 4, 2024 •
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@kamuik16
Copy link
Contributor Author

kamuik16 commented Jun 4, 2024

I have read the CLA Document and I hereby sign the CLA

@kamuik16
Copy link
Contributor Author

kamuik16 commented Jun 4, 2024

recheck

github-actions bot added a commit that referenced this pull request Jun 4, 2024
@github-actions
@kamuik16 has signed the CLA in #154
cd56ce9
fedgiac
fedgiac reviewed Jun 4, 2024
View reviewed changes
Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!
The test migration has been done well. I just have some nit comments before merging.

test/reader/StorageReadable.t.sol Outdated Show resolved Hide resolved
test/reader/StorageReadable.t.sol Outdated Show resolved Hide resolved
test/reader/StorageReadable.t.sol Outdated Show resolved Hide resolved
test/reader/StorageReadable.t.sol Outdated Show resolved Hide resolved
test/reader/StorageReadable.t.sol Outdated Show resolved Hide resolved
test/reader/StorageReadable.t.sol Outdated Show resolved Hide resolved
test/reader/StorageReadable.t.sol Outdated Show resolved Hide resolved
@kamuik16
Copy link
Contributor Author

kamuik16 commented Jun 4, 2024

hey @fedgiac! I have implemented all of your suggestions. Thanks!

@kamuik16 kamuik16 requested a review from fedgiac June 4, 2024 16:09
@fedgiac
Copy link
Contributor

fedgiac commented Jun 4, 2024

Thank you! We'll get back to you once we're settled on the license, but otherwise it's ready to merge.

@mfw78
Copy link
Contributor

mfw78 commented Jun 5, 2024

First, thanks for the contribution! 🚀

In preparation for the merge of #155 can you also please update the pragma (the intention is all tests / CI/CD will run against 0.8.26 so we're not re-implementing aspects of tests that we just implement once the pragma bump happens 😄

@kamuik16
Copy link
Contributor Author

kamuik16 commented Jun 5, 2024

First, thanks for the contribution! 🚀

In preparation for the merge of #155 can you also please update the pragma (the intention is all tests / CI/CD will run against 0.8.26 so we're not re-implementing aspects of tests that we just implement once the pragma bump happens 😄

done!

fedgiac
fedgiac reviewed Jun 5, 2024
View reviewed changes
Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

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

The license has been settled to LGPL. Once the license is modified and changes from main are merged into here we can merge this PR.

test/reader/StorageReadable.t.sol Outdated Show resolved Hide resolved
@kamuik16 kamuik16 requested a review from fedgiac June 5, 2024 15:05
@fedgiac
Copy link
Contributor

fedgiac commented Jun 5, 2024

This branch is out of date with main, otherwise everything looks good.

mfw78 and others added 2 commits June 5, 2024 20:48
@mfw78 @fedgiac @kamuik16
chore: bump pragma (cowprotocol#155)
3c0d01c 
## Description

This PR bumps the pragma to support up-to-and-including solidity
(`0.8.26`). This allows the foundry migration taking place to have tests
written in solc `0.8.x`, reducing redundancy / effort with writing the
associated test contracts.

It is important that the `bytecode` generated for production still be
compliant with `0.7.6` (save for potential issues associated with the
`metadata` hash appended by `solc`). To facilitate this, the `test`
pipeline uses a multi-version matrix strategy to ensure that the
contracts remain able to be built with `0.7.6`.

A minor miscellaneous patch is also included for VS Code's solidity
configuration, reducing configuration size.

## Test Plan

1. Verify `test` CI/CD runs `0.7.6` and `0.8.26` matrices.
2. Verify that the `ci` CI/CD runs (`hardhat`) and completes
successfully.

## Related Issues

Related: cowprotocol#106

---------

Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
@kamuik16
merge main
49e545a
@kamuik16
Copy link
Contributor Author

kamuik16 commented Jun 5, 2024 •
edited
Loading

This branch is out of date with main, otherwise everything looks good.

aah, i guess i got those commits in my PR. shall I make a new PR?

@fedgiac
Copy link
Contributor

fedgiac commented Jun 5, 2024

I merged the main branch into your PR from the web interface and this took care of the code from the other commits. We don't mind about git history in the PR since it's going to be squashed into main anyway.
This is good to merge now, thanks for your contribution!

@fedgiac fedgiac merged commit a4deb4f into cowprotocol:main Jun 5, 2024
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2024
@kamuik16 kamuik16 deleted the kamuik16/test branch June 5, 2024 16:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fedgiac fedgiac fedgiac 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.

tests(core): StorageReadable
3 participants
@kamuik16 @fedgiac @mfw78