Skip to content

E2e migration update#253

Open
kaze-cow wants to merge 4 commits intoe2e-migrationfrom
e2e-migration-update
Open

E2e migration update#253
kaze-cow wants to merge 4 commits intoe2e-migrationfrom
e2e-migration-update

Conversation

@kaze-cow
Copy link
Copy Markdown
Contributor

@kaze-cow kaze-cow commented Dec 28, 2025

Description

Forking off the original e2e-migration branch, finish the E2E tests migration. This PR has been pending for a while and needed some updates based on the latest state of solidity/plans for the future of the protocol, so its been updated/adapted.

Concretely, this branch differs from the original branch by:

  • updating OpenZeppelin to 5.5.0 (the latest), which supports solidity 0.8, simplifying some aspects of the tests such as delpoyment of openzepplin test contracts
  • remove tests related to balancer/internal balances functionality. This will not be used in the next version of the protocol.
  • updating the code to work with the latest foundry warnings cleaned
  • ran forge fmt
  • update hardhat (this wasnt directly related to any issues with the codebase, but it was causing yarn to fail install on my ARM hardware and updating seems to have no noticable negative effect at this time)

The actual diff between original e2e testing brnach can be seen here (individual commits viewing is helpful due to the forge fmt): e2e-migration...e2e-migration-update

The previous branch was pretty scant on documentation and review, and never received a review in the first place. The original reviewers are no longer here, and some lint fixes from upstream foundry are needed, so may we just start again with this branch?

Test Plan

The new tests can be verified with FORK_URL=<url of mainnet> forge test. There are no more tests in hardhat. We may consider removing hardhat entirely?

Related Issues

#226

* remove balancer tests (we will be actively removing balancer support itself by the next release)
* update to use latest openzepplin (actually supports 0.8)
* remove the foundry warnings error (temp so that we can get things to compile)
@kaze-cow kaze-cow requested a review from a team as a code owner December 28, 2025 05:26
@socket-security
Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updatedts-node@​10.9.1 ⏵ 10.9.297 +110010082100
Updatedhardhat@​2.13.1 ⏵ 2.28.09410092 +496 -180
Updated@​openzeppelin/​contracts@​3.4.0-solc-0.7 ⏵ 5.5.0100100 +75100 +694 +3100

View full report

@socket-security
Copy link
Copy Markdown

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action Severity Alert  (click "▶" to expand/collapse)
Warn High
Obfuscated code: npm safer-buffer is 94.0% likely obfuscated

Confidence: 0.94

Location: Package overview

From: ?npm/hardhat@2.28.0npm/safer-buffer@2.1.2

ℹ Read more on: This package | This alert | What is obfuscated code?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/safer-buffer@2.1.2. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

there are probably some dumb things here and there but this serves as a starting point

now moving onto coverage, there are some things missing
Copy link
Copy Markdown
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

It doesn't build for me. For example, I can't find this file:
Image

@anxolin
Copy link
Copy Markdown
Contributor

anxolin commented Apr 13, 2026

Also, @kaze-cow please remember to set the correct base branch to see the actual changes. You are based in e2e-migration. It's good to always check the changes in your own PRs. In this case, it would have shown 7K changes.

@anxolin anxolin changed the base branch from main to e2e-migration April 13, 2026 15:08
Copy link
Copy Markdown
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Should we cleanup and get these merged?

forge build fails for me

 --> src/contracts/GPv2Settlement.sol:7:1:
  |
7 | import {IGPv2Settlement} from "./interfaces/IGPv2Settlement.sol";
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Can we make sure tests are passing?

/// protection mixed in so that signed orders are only valid for specific
/// GPv2 contracts.
bytes32 public immutable domainSeparator;
bytes32 public immutable DOMAIN_SEPARATOR;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes the public getter function name, this affects the ABI, is this intended?
and also should it be in e2e migration PRs or should it be an independent one?

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.

2 participants