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

Apply fixes based on code review #11

Merged
merged 6 commits into from
Jun 11, 2024

Conversation

miguelmtzinf
Copy link

@miguelmtzinf miguelmtzinf commented Jun 4, 2024

Copy link

height bot commented Jun 4, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

4 similar comments
Copy link

height bot commented Jun 4, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Copy link

height bot commented Jun 4, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Copy link

height bot commented Jun 4, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Copy link

height bot commented Jun 4, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@miguelmtzinf miguelmtzinf marked this pull request as ready for review June 6, 2024 10:45
Copy link

github-actions bot commented Jun 6, 2024

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

Copy link

github-actions bot commented Jun 6, 2024

LCOV of commit ffc4c20 during Solidity Foundry #6

Summary coverage rate:
  lines......: 94.7% (1122 of 1185 lines)
  functions..: 91.7% (266 of 290 functions)
  branches...: 87.5% (462 of 528 branches)

Files changed coverage rate: n/a

⛔ The code coverage is too low: 94.68. Expected at least 98.5.

@@ -58,9 +59,4 @@ contract UpgradeableBurnMintTokenPool is VersionedInitializable, UpgradeableBurn
function REVISION() public pure virtual returns (uint256) {

Choose a reason for hiding this comment

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

while this doesn't hurt it also doesn't help and is a bit useless when using Initializable imo. I'd rather remove it.

Copy link
Author

Choose a reason for hiding this comment

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

removed, ty!

Copy link

github-actions bot commented Jun 7, 2024

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

Copy link

github-actions bot commented Jun 7, 2024

LCOV of commit 748d036 during Solidity Foundry #7

Summary coverage rate:
  lines......: 94.7% (1120 of 1183 lines)
  functions..: 91.7% (264 of 288 functions)
  branches...: 87.5% (462 of 528 branches)

Files changed coverage rate: n/a

⛔ The code coverage is too low: 94.67. Expected at least 98.5.

@AsafHochstadtCertora
Copy link

After reviewing the changes, the fixes seem ok to me

@miguelmtzinf miguelmtzinf merged commit 7c777f9 into feat/gho-token-pool Jun 11, 2024
25 of 45 checks passed
@miguelmtzinf miguelmtzinf deleted the fix/gho-token-pool-fixes branch June 11, 2024 08:28
miguelmtzinf added a commit that referenced this pull request Jun 11, 2024
* feat: Add custom upgradeable TokenPool contracts

* test: Add unit and e2e tests (#4)

* test: Add unit and e2e tests

* test: Add tests for bridge limit config

* fix: Fix foundry toml

* feat: Add Bridge limit admin (#6)

* test: Add unit and e2e tests

* test: Add tests for bridge limit config

* fix: Fix foundry toml

* feat: Add bridge limit admin

* fix: Rename error

* doc: Add caution docs to setBridgeLimit

* fix: Fix github action dir name

* fix: Add code diffs between custom token pools and standard implementation

* fix: Directory restructure

* fix: Fix README

* fix: Remove unneccesary files

* fix: fix README

* test: Refactor invariant tests

* test: Refactor tests

* fix: Rename test files

* Certora review (#10)

* fix: Apply fixes based on code review (#11)

* fix: Use modern version of Initializable

* fix: Add event emission on bridge limit admin update

* fix: Rebuild GHO diffs

* fix: Remove unneeded revision getter

* ci: Remove unneeded ci actions (#12)

* fix: Use modern version of Initializable

* fix: Update diffs

* fix: Add event emission on bridge limit admin update

---------

Co-authored-by: Nissan Levi <[email protected]>
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