INT-402: NativeOFTAdapterUpgradeable - #1591
Conversation
| // Skip native adapter assertions in fee test suite | ||
| } | ||
|
|
||
| function test_native_oft_adapter_debit() public virtual override { |
There was a problem hiding this comment.
I'm not a fan of this solution for testing. I think it leaks problems too much to the test suite that derives from it.
It seems each time we will use OFT.t.sol as a base we will have to manually skip native OFT adapter tests?
I think we should instead create a new test file specifically for NativeOFTAdapter or NativeOFTAdapterUpgradeable so we don't have to call vm.skip(true) in any of test suites that derives from OFT.t.sol.
New test file can derive from OFT.t.sol.
There was a problem hiding this comment.
what we need is a base test framework for ugradeable and then there is a new test file that tests ONLY that OApp.
reference: https://github.com/LayerZero-Labs/devtools/tree/feat/ovault-evm/packages/ovault-evm/test/composer
which creates a base, and then you implement Unit, E2E, Upgradeable Unit, etc
| constructor( | ||
| uint8 _localDecimals, | ||
| address _lzEndpoint | ||
| ) NativeOFTAdapterUpgradeable(_localDecimals, _lzEndpoint, address(0)) {} |
There was a problem hiding this comment.
disableInitializers in constructor of the mock
| ) | ||
| ); | ||
|
|
||
| dNativeOFTAdapter = NativeOFTAdapterUpgradeableMock( |
There was a problem hiding this comment.
Please use separate file for native OFT adapter tests or something that doesn't have to be vm.skipped if something derives from the test suite
| // @notice Codec to convert NativeOFTAdapterUpgradeable to NativeOFTAdapterUpgradeableMock in a consistent, readable manner. | ||
| // @dev For testing purposes only. | ||
| library NativeOFTAdapterUpgradeableMockCodec { | ||
| function asNativeOFTAdapterUpgradeableMock(NativeOFTAdapterUpgradeable _oft) internal pure returns (NativeOFTAdapterUpgradeableMock) { |
| assertEq(dNativeOFTAdapter.approvalRequired(), false); | ||
| } | ||
|
|
||
| function test_oftVersion() public view { |
There was a problem hiding this comment.
why is this change required?
| uint256 requiredMsgValue = _fee.nativeFee + _removeDust(_sendParam.amountLD); | ||
| if (msg.value != requiredMsgValue) { | ||
| revert IncorrectMessageValue(msg.value, requiredMsgValue); | ||
| } |
There was a problem hiding this comment.
this expects that the user uses a sendParam.amountLD that would result in no dust. why cant the user send extra native and it get refunded to them later at the refund address?
There was a problem hiding this comment.
this is how NativeOFTAdapter (non-upgradeable) operates already, I don't think we should change this in upgradeable version. we should change in both if we want to change it
in my legacy implementation I have also allowed refunds, here you need to pass very precise amounts which I think can be a little bit worse UX during gas spikes: revert instead of refund
There was a problem hiding this comment.
yeah we should change it in both places. revert if not exact is worse UX
shankars99
left a comment
shankars99
left a comment
There was a problem hiding this comment.
not a fan of the send()
|
Closing this PR as Plume has stated it is not required to have upgradeable contracts |
3 participants
In this PR:
OFTCoreUpgradeable