Upgrade MOVE to OFT - #1298
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ntlabsxyz/movement into primata/contract-pipeline
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades the MOVE token to inherit from OFTUpgradeable and adds native OFT (Omnichain Fungible Token) capabilities for cross-chain functionality. The upgrade also includes significant access control changes by transferring admin roles from the old Movement Foundation multisig to Movement Labs Operations multisig and burning bridge balances.
Key changes:
- Implements MOVETokenV2 with LayerZero OFT capabilities for cross-chain transfers
- Updates access control by granting DEFAULT_ADMIN_ROLE to Movement Labs and revoking it from old Foundation
- Burns deprecated bridge balances during upgrade initialization
- Adds comprehensive test coverage for the upgrade process and OFT functionality
Reviewed Changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| MOVETokenV2.sol | New implementation inheriting from MOVEToken and OFTUpgradeable with upgrade initialization |
| MOVEToken.sol | Makes decimals() function virtual to support inheritance |
| MOVETokenV2.t.sol | Comprehensive test suite covering upgrade process, OFT features, and LayerZero integration |
| Various test files | New test files for token components (base, custodian, locked tokens, staking) |
| Configuration files | Updated deployment addresses and build settings |
| Git submodules | Added LayerZero and devtools dependencies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "minDelay": "172800", | |||
| "minDelay": 172800, | |||
There was a problem hiding this comment.
The minDelay value changed from a string "172800" to a number 172800. While this works functionally, ensure all consuming code properly handles both string and numeric formats to avoid parsing errors.
| "minDelay": 172800, | |
| "minDelay": "172800", |
There was a problem hiding this comment.
is this with respect to the recent string vs value "update"?
|
This PR combines large changes that have nothing to do with the current MOVE token upgrade to V2. Hence the description of this PR is incomplete. In particular the description does not describe, why there are many changes and indeed creations of files related to the current MoveToken. ( I take it that you forked of another branch, or copied in files from another branch ). I suggest to split this into two PRs.
|
#1285 it's already here |
0xmovses
left a comment
There was a problem hiding this comment.
Unable to run the tests, perhaps a step is missing?
⋊> ~/m/movement on upgrade-move-oft ◦ cd protocol-units/settlement/mcr/contracts 15:16:36
⋊> ~/m/m/p/s/m/contracts on upgrade-move-oft ◦ forge test ./test/token/MOVETokenV2.t.sol --rpc-url $ETHEREUM_MAINNET_RPC -vvv 15:16:42
error: unexpected argument './test/token/MOVETokenV2.t.sol' found
Usage: forge test [OPTIONS]
For more information, try '--help'
No, that's correct, can you try running the MOVETokenV2 test by yourself? You can do it a few ways |
|
CMD in the PR description should result in a successful test. You can either update the cmds or fix whatever is wrong. Thanks! |
|
Not sure what's happening here, I just pulled latest and ran Can you fix the cmd so I'm able to run the tests? |
|
This now has many merge conflicts @Primata |
apenzk
left a comment
There was a problem hiding this comment.
The MOVETokenV2.t.sol looks great with the comments and seems easy accessible. All tests work.
| * @param _revoke The address of the address to revoke role. | ||
| * @param _burned Burns circulation supply on Ethereum. | ||
| */ | ||
| function initialize(address _delegate, address _revoke, address[] calldata _burned) external reinitializer(2) { |
There was a problem hiding this comment.
_delegate is movement labs operations multisig
_revoke is movement foundation multisig
| function initialize(address _delegate, address _revoke, address[] calldata _burned) external reinitializer(2) { | ||
| __OFTCore_init(_delegate); | ||
| __Ownable_init_unchained(_delegate); | ||
| _grantRole(DEFAULT_ADMIN_ROLE, _delegate); |
There was a problem hiding this comment.
movement labs ops receives DEFAULT_ADMIN_ROLE
| __OFTCore_init(_delegate); | ||
| __Ownable_init_unchained(_delegate); | ||
| _grantRole(DEFAULT_ADMIN_ROLE, _delegate); | ||
| _revokeRole(DEFAULT_ADMIN_ROLE, _revoke); |
There was a problem hiding this comment.
Previous movement foundation multisig had the DEFAULT_ADMIN_ROLE, here we intend to revoke it.
| _grantRole(DEFAULT_ADMIN_ROLE, _delegate); | ||
| _revokeRole(DEFAULT_ADMIN_ROLE, _revoke); | ||
| for (uint256 i = 0; i < _burned.length; i++) { | ||
| _burn(_burned[i], balanceOf(_burned[i])); |
There was a problem hiding this comment.
Specific addresses like the old bridge contract and liquidity meant to be released on the Movement side to initial validator rewards get their supply burned.
| /** | ||
| * @dev Disables potential implementation exploit | ||
| */ | ||
| constructor(address _endpoint) OFTUpgradeable(_endpoint) {_disableInitializers();} |
There was a problem hiding this comment.
endpoint is defined in the implementation. Any new upgrade would need the new implementation to once again specify the endpoint.
| import {MOVEToken} from "./MOVEToken.sol"; | ||
| import {OFTUpgradeable, ERC20Upgradeable} from "@layerzerolabs/oft-evm-upgradeable/contracts/oft/OFTUpgradeable.sol"; | ||
|
|
||
| contract MOVETokenV2 is MOVEToken, OFTUpgradeable { |
There was a problem hiding this comment.
here we inherit from MOVEToken to avoid collision.
Summary
protocol-units,scripts.Upgrades MOVE token to inherit OFTUpgradeable and become a native OFT.
It also:
Changelog
Testing
Outstanding issues
Full upgrade procedure