[UNTRACKED] V2 #57
Merged
[UNTRACKED] V2 #57
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nd tests (circlefin#10) As discussed offline with @walkerq, breaking apart STABLE-6895 into 2 parts: 1) Extract base token messenger behavior and tests, copied from v1, into a separate type (`BaseTokenMessenger`). This base type handles adding / removing remote token messengers, the local minter, and encodes the local message transmitter / version. `TokenMessengerV2` derives from this type and layers on the v2-specific, messaging-layer differences. I think this is more flexible for a potential v3 and is a natural separation of concerns, but definitely open to further discussion! 2) (Future) integrate hooks, the first V2-specific functionality, into `TokenMessengerV2`. Some misc callouts: - Intention is leave the v1 contract code unmodified (optional: in the future we can refactor `TokenMessenger.t.sol` to use these shared baseTokenMessenger tests to avoid duplication). - Added some additional Ownable2Step test cases into `TestUtils.sol`. - Copied over the v1 unit tests from `TokenMessenger.t.sol`, but injected more fuzzable inputs into some of the test cases. Further callouts below in-line.
…irclefin#13) ## Summary This implements `depositForBurnWithHook` on the relay side. This includes: - Updating `TokenMessengerV2` with a new `depositForBurnWithHook` function - Validating inputs - Using `BurnMessageV2` to format the burn message correctly (note: the Iris-inserted fields are omitted). - Calling message transmitter with the formatted message. It does _not_ cover: - MessageTransmitt-ing - Handling receive messages or executing hooks ^ will be taken care of in part 3. ## Testing This extends the `TokenMessengerV2` tests, using many of the same test cases from `TokenMessenger.t.sol`. Some callouts and questions below in-line. ## Discussion Open to any and all ideas for further code re-use between v1 and v2. The principle has been to leave v1 as-is, so re-use as focused on implementations and tests, vs strictly sharing code.
As described by [STABLE-7247](https://circlepay.atlassian.net/browse/STABLE-7247?atlOrigin=eyJpIjoiY2ZjMzRmZGI5Y2Y5NDgyN2JhNjJlM2E5ZGU5NjVlOWMiLCJwIjoiaiJ9): - Copied over deploy scripts 1-4 from evm-bridge-contracts - Added tests to verify deployed contracts' parameters [STABLE-7247]: https://circlepay.atlassian.net/browse/STABLE-7247?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
As described by [STABLE-7268](https://circlepay.atlassian.net/browse/STABLE-7269?atlOrigin=eyJpIjoiZjgzMjBiMjEyMGRhNDc1OGFkNWQxM2NlNzhhMDU3YjUiLCJwIjoiaiJ9): - Updated Dockerfile to (2024-03-15) nightly Foundry build, which is the last version that still builds locally on Mac. Version is still 0.2.0. **NOTE: Docker images that are even newer do not support arm64 (macos) and only supports linux/amd64.** There is an open task to address this issue [here](foundry-rs/foundry#7512). - Updated lib/forge-std submodule version to v1.9.2. As a result, changed all tests to `pragma abicoder v2` for compatibility with forge-std/Test.sol. - Fixed tests that were failing due to vm cheatcodes registering as a call, thus catching vm.expectEmit(). - Fixed a test that failed due to OOG. ## [REFERENCE] AbiCoder V2 fixes/changes since solidity 0.7.6 ### 0.8.15 * ABI Encoder: When encoding an empty string coming from storage do not add a superfluous empty slot for data. ### 0.8.14 * ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases. ### 0.8.10 * Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist. ### 0.8.4 * ABI Decoder V2: For two-dimensional arrays and specially crafted data in memory, the result of abi.decode can depend on data elsewhere in memory. Calldata decoding is not affected. ### 0.8.0 * General: Enable ABI coder v2 by default. [STABLE-7268]: https://circlepay.atlassian.net/browse/STABLE-7268?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: ams9198 <[email protected]>
A prior PR merge added new v2 script tests that were not updated with the `pragma abicoder v2` specification.
…efin#15) ## Summary 1) Adds the `depositForBurn` external function on `TokenMessengerV2` + tests on the relay side, building on patterns in the past PRs + v1. 2) Updates `fee` --> `maxFee` per the latest [in the designs](https://circlepay.atlassian.net/wiki/spaces/~5ab29c9a45a2562a4c64de77/pages/1476788228/CCTP+V2+Contract+Requirements). Part 2) will fill out the `MessageTransmitterV2` implementation and the receiving side. Breaking this into 2 parts to manage PR size. ## Tests Add tests for `depositForBurn` validations and event emission. ## Misc Left 1 callout below.
…irclefin#18) ## Summary This continues on Part 1, building on `depositForBurn` and `depositForBurnWithHook` TokenMessenger implementations to build out the `MessageTransmitterV2.sendMessage()`. Changes: - Add MessageTransmitter.receiveMessage, along with Rescuable, Pausable inheritance and tests - Add MessageV2 and BurnMessageV2, now that source <> destination chain format questions have been resolved, along with tests - Add some missing Ownable, Rescuable, and Pausable tests around access control ## Testing This adds new tests to `BurnMessageV2.t.sol`, `MessageV2.t.sol`, and `MessageTransmitterV2.t.sol`, along with small tweaks to v1 test files to incorporate the small changes made to the shared Rescuable, Ownable, Pausable tests.
As per [STABLE-7248](https://circlepay.atlassian.net/browse/STABLE-7248?atlOrigin=eyJpIjoiMGI5ZDI3OTc2ZWZhNDIzYjg5NTQ2YzQ5YzViYTgzMzgiLCJwIjoiaiJ9): - Created new `ProxyFactory` that needs to be deployed traditionally via CREATE - Added deploy script and tests for `ProxyFactory`. ## NOTE This PR does not yet integrate v1/v2 deployment scripts. The v2 contracts will need to be updated to use an initialize pattern. [STABLE-7248]: https://circlepay.atlassian.net/browse/STABLE-7248?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
…in MessageTransmitterV2 (circlefin#19) ### Summary This adds deterministic nonce handling in MessageTransmitterV2. It completes the `receiveMessage` implementation and adds a bunch of tests derived from the v1 testcases. A few callouts below in-line. The vast majority of the diff is in tests and license headers for new files--the actual contract changes are fairly compact. ### Testing See `v2/MessageTransmitterV2.t.sol` for test additions. Please scrutinize the test cases and compare them to the v1 tests in `MessageTransmitter.t.sol` and see if anything is missing. I opted to focus on fuzzing additional attributes and using mocks to focus on the contract under test.
### Summary In PR 19 we added a bunch of fuzz-based tests for `MessageTransmitterV2`: circlefin/evm-cctp-contracts-private#19. I realized that we were not capturing the condition where: `destinationCaller is 0, so we should use random addresses to call receiveMessage()`. Instead, we were always just spoofing the 0-address due to this line: https://github.com/circlefin/evm-cctp-contracts-private/blob/220b071ecf7d3c221dfd71ad6f8802b25611ac89/test/v2/MessageTransmitterV2.t.sol#L1039, which is equivalent to the tests for non-zero value destinationCallers. ### Changes This updates the fuzz tests to take in a random caller. If the destinationCaller is set, we spoof that address, otherwise we call with a random fuzzed address.
### Summary Adds a Denylist to TokenMessenger. Updated the ABI to align with the Paymaster PR: circlefin/smart-wallet-contracts-private#155, though the implementations are slightly difference. The requirements are to check the denylist on `depositForBurn` and `depositForBurnWithHook` for both the `msg.sender` and `tx.origin` addresses. This is pretty straightforward, but some callouts are down below. ### Testing Added dedicated tests using a mock Denylistable, and unit tests for TokenMessengerV2 and BaseTokenMessenger.
### Summary This adds an `Initializable` type and updates `TokenMessengerV2` and `MessageTransmitterV2` to use it. It also adds initializers to both contracts. Some implementation notes: - The `Initializable` type is forked from OZ, and then modified for Solidity 0.7.6 (no custom errors) and stripped down of features that we're not using. - Please scrutinize the fork changes to `Initializable`. I'm not wed to using this type for our purposes. - Had to create internal setters for Pausable, Rescuable, and Attestable to allow the initializers to set those values. Some further callouts down below ### Testing Added tests for Initializable + the initializer function additions to the TokenMessenger and MessageTransmitter, and also simulated an upgrade to mock v3 versions.
### Summary I ran `forge coverage` and noticed 2 tiny gaps. This PR fixes one of them. The other one is due to[ this guard statement](https://github.com/circlefin/evm-cctp-contracts-private/blob/40a2e35a518cad1664ebc94bb951c5b43f4f13c7/src/v2/AdminUpgradableProxy.sol#L71) never being hit; however, it's not actually possible to test that. However, all other files used in v2 have 100% coverage across all branches. The `Ownable` coverage is sorta misleading, since we don't use that contract directly, and instead via `Ownable2Step`. After this PR: `Ran 21 test suites in 650.11s (2864.13s CPU time): 415 tests passed, 0 failed, 0 skipped (415 total tests)` | File | % Lines | % Statements | % Branches | % Funcs | |------------------------------------------------|-------------------|--------------------|------------------|------------------| | scripts/v2/1_deploy.s.sol | 100.00% (53/53) | 100.00% (58/58) | 100.00% (0/0) | 100.00% (8/8) | | scripts/v2/2_setupSecondAttester.s.sol | 100.00% (12/12) | 100.00% (12/12) | 100.00% (0/0) | 66.67% (2/3) | | scripts/v2/3_setupRemoteResources.s.sol | 100.00% (20/20) | 100.00% (22/22) | 100.00% (0/0) | 100.00% (4/4) | | scripts/v2/4_rotateKeys.s.sol | 100.00% (27/27) | 100.00% (27/27) | 100.00% (0/0) | 40.00% (2/5) | | src/MessageTransmitter.sol | 100.00% (51/51) | 100.00% (70/70) | 100.00% (21/21) | 100.00% (9/9) | | src/TokenMessenger.sol | 100.00% (77/77) | 100.00% (95/95) | 100.00% (38/38) | 100.00% (18/18) | | src/TokenMinter.sol | 100.00% (22/22) | 100.00% (25/25) | 100.00% (12/12) | 100.00% (9/9) | | src/messages/BurnMessage.sol | 100.00% (9/9) | 100.00% (13/13) | 100.00% (4/4) | 100.00% (7/7) | | src/messages/Message.sol | 100.00% (15/15) | 100.00% (18/18) | 100.00% (4/4) | 100.00% (12/12) | | src/messages/v2/AddressUtils.sol | 100.00% (2/2) | 100.00% (2/2) | 100.00% (0/0) | 100.00% (2/2) | | src/messages/v2/BurnMessageV2.sol | 100.00% (12/12) | 100.00% (20/20) | 100.00% (4/4) | 100.00% (10/10) | | src/messages/v2/MessageV2.sol | 100.00% (15/15) | 100.00% (19/19) | 100.00% (4/4) | 100.00% (12/12) | | src/roles/Attestable.sol | 100.00% (38/38) | 100.00% (46/46) | 100.00% (26/26) | 100.00% (14/14) | | src/roles/Ownable.sol | 77.78% (7/9) | 77.78% (7/9) | 50.00% (2/4) | 83.33% (5/6) | | src/roles/Ownable2Step.sol | 100.00% (8/8) | 100.00% (9/9) | 100.00% (2/2) | 100.00% (4/4) | | src/roles/Pausable.sol | 100.00% (11/11) | 100.00% (11/11) | 100.00% (6/6) | 100.00% (7/7) | | src/roles/Rescuable.sol | 100.00% (7/7) | 100.00% (7/7) | 100.00% (4/4) | 100.00% (5/5) | | src/roles/TokenController.sol | 100.00% (21/21) | 100.00% (25/25) | 100.00% (12/12) | 100.00% (9/9) | | src/roles/v2/Denylistable.sol | 100.00% (15/15) | 100.00% (16/16) | 100.00% (7/7) | 100.00% (9/9) | | src/v2/AdminUpgradableProxy.sol | 100.00% (16/16) | 100.00% (17/17) | 83.33% (5/6) | 100.00% (9/9) | | src/v2/BaseTokenMessenger.sol | 100.00% (47/47) | 100.00% (52/52) | 100.00% (28/28) | 100.00% (16/16) | | src/v2/Create2Factory.sol | 100.00% (4/4) | 100.00% (4/4) | 100.00% (0/0) | 100.00% (3/3) | | src/v2/Initializable.sol | 100.00% (29/29) | 100.00% (35/35) | 100.00% (11/11) | 100.00% (8/8) | | src/v2/MessageTransmitterV2.sol | 100.00% (45/45) | 100.00% (58/58) | 100.00% (31/31) | 100.00% (6/6) | | src/v2/TokenMessengerV2.sol | 100.00% (45/45) | 100.00% (52/52) | 100.00% (24/24) | 100.00% (9/9) | | src/v2/TokenMinterV2.sol | 100.00% (6/6) | 100.00% (7/7) | 100.00% (6/6) | 100.00% (2/2) | | test/TestUtils.sol | 100.00% (194/194) | 100.00% (211/211) | 100.00% (0/0) | 11.76% (2/17) | | test/mocks/MockInitializableImplementation.sol | 100.00% (5/5) | 100.00% (7/7) | 100.00% (0/0) | 100.00% (7/7) | | test/mocks/MockMintBurnToken.sol | 100.00% (31/31) | 100.00% (31/31) | 66.67% (8/12) | 100.00% (11/11) | | test/mocks/MockReentrantCaller.sol | 100.00% (12/12) | 100.00% (17/17) | 75.00% (3/4) | 100.00% (5/5) | | test/mocks/MockTokenMessenger.sol | 100.00% (4/4) | 100.00% (6/6) | 100.00% (3/3) | 100.00% (2/2) | | test/mocks/v2/MockDenylistable.sol | 100.00% (1/1) | 100.00% (1/1) | 100.00% (0/0) | 100.00% (1/1) | | test/mocks/v2/MockMessageTransmitterV3.sol | 100.00% (2/2) | 100.00% (2/2) | 100.00% (0/0) | 100.00% (3/3) | | test/mocks/v2/MockProxyImplementation.sol | 75.00% (3/4) | 75.00% (3/4) | 100.00% (0/0) | 75.00% (3/4) | | test/mocks/v2/MockReentrantCallerV2.sol | 100.00% (2/2) | 100.00% (4/4) | 100.00% (0/0) | 100.00% (2/2) | | test/mocks/v2/MockTokenMessengerV3.sol | 100.00% (1/1) | 100.00% (1/1) | 100.00% (0/0) | 100.00% (2/2) | | test/scripts/ScriptV2TestUtils.sol | 100.00% (56/56) | 100.00% (60/60) | 100.00% (0/0) | 100.00% (4/4) | | Total | 99.68% (925/928) | 99.72% (1070/1073) | 97.07% (265/273) | 92.05% (243/264) |
circlefin#33) ### Summary See discussion here: https://circlefin.slack.com/archives/C07L385TX7B/p1729099569014349 We'd like the ABIs across the projects to be identical.
Update `deployAndCall` to `deployAndMultiCall` to allow for atomic bundling of all proxy deployment steps in a singular transaction. - Arrayified `data` param. - Updated tests. V2 deployment process doc: https://docs.google.com/document/d/1ayFWGNVmaO6jlLwgZsa0Yk-490CTBdiFASuY4jJzkE0/edit?pli=1&tab=t.0#heading=h.r8mn3sdqq2kb
# Overview Updated deployment scripts and process documentation for V2 contracts as [documented](https://docs.google.com/document/d/1ayFWGNVmaO6jlLwgZsa0Yk-490CTBdiFASuY4jJzkE0/edit?usp=sharing). # Changes - Added new scripts for deploying and configuring the V2 contracts. - Added tests for the new scripts. - Updated the readme and Makefile for V2 deployment process. # Tests - Added new script tests. - Manually followed the deployment process locally. # Review - Help take a look to make sure all the env vars make sense. (Especially regarding all the owners/pks.) - Optionally, run through the process locally.
…irclefin#47) ### Summary Migrate from Slither to Mythril for static analysis ### Detail - update Makefile command and update CI - remove Slither relevant configs and add Mythril config - update Readme -- *story*: https://circlepay.atlassian.net/browse/STABLE-7559
Summary: Introduces a new Github Action workflow to integrate the Olympix Static Analysis tool for Solidity code scanning. - Adds .github/workflows/ci-olympix.yml that uses a reusable workflow from [security-seceng-templates](https://github.com/circlefin/security-seceng-templates/blob/master/.github/workflows/olympix_scan.yml) - Workflow is triggered on every pull request as well as scheduled on a weekly basis.(Monday)
…efin#48) ### Summary ~Note: Leaving as a draft for now for early feedback; will flip it to a regular PR later.~ This adds fee collection support to the finalized message handling codepath in TokenMessengerV2. This is helpful to collect fees on re-signed messages that have expired, but originally consumed an allowance. Changes: - Unpack and validate the `feeExecuted` parameter on both finalized and unfinalized message handling codepaths - Unpack and validate the `expirationBlock` parameter on both finalized and unfinalized message handling codepaths. Note that for re-signed messages, `expirationBlock` should be set to 0 at launch. ### Testing The first commit adds failing tests; the 2nd commit updates the implementation such that they pass. The latter commits refactor slightly and add an additional test. To test, run the unit and integration tests.
… version to use (circlefin#53) ## Summary Allow users of this repository as a submodule to override the foundry version to use ## Detail This repository is used as a submodule elsewhere to build evm contracts, at the same time these other repositories install said contracts using a locally managed foundry version. when the versions don't align the builds can sometimes fail causing the need to cascade update the foundry version everywhere. Allowing the Dockefile to use a build argument means other downstream repositories can update the version at their own pace without having to update this repository first. ## Testing covered by existing tests ## Documentation **Story:** [UNTRACKED](https://circlepay.atlassian.net/browse/)
…sing from env (circlefin#55) - Add flag in DeployProxiesV2 script to determine whether to read `remoteTokenMessengerAddresses` from env variable. - Add logic to use deterministic Create2 address calculation if not reading from env. - Update UT to test the false case. True case will most likely not get used. - TODO: Figure out a way to set flag to true based on test selector before `setUp()` is run to test the true case
| @@ -0,0 +1,79 @@ | |||
| /* | |||
| * Copyright (c) 2024, Circle Internet Financial Limited. | |||
| * | |||
There was a problem hiding this comment.
Do we need put SPDX-License-Identifier: Apache-2.0 here like https://github.com/circlefin/evm-cctp-contracts/pull/57/files#diff-592591b43550ff2b95b0243de7d9becbd8bdbb395ac1e5982785069cf5fc09fdR4?
walkerq
approved these changes
Jan 23, 2025
hsinghgrewal
approved these changes
Jan 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Faster-than-finality cross-chain USDC transfers