-
Notifications
You must be signed in to change notification settings - Fork 222
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
Migrate HardHat tests to DS-tests (Solidity tests) #601
Changes from 15 commits
6bb15f7
93d1571
3d0dc81
11c8d0b
84a27c4
ba3cde5
04b6924
926bd97
d9dd232
1e1044e
59b1c79
82c47ae
37a2737
6770605
b315b50
aaf472c
9bc25bf
9f2e2e9
83312f8
b1e193a
db2cde0
0b13065
e049a0d
f3e7fde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,12 @@ import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; | |
import "./AllowList.sol"; | ||
import "./INativeMinter.sol"; | ||
|
||
address constant MINTER_ADDRESS = 0x0200000000000000000000000000000000000001; | ||
address constant BLACKHOLE_ADDRESS = 0x0100000000000000000000000000000000000000; | ||
|
||
contract ERC20NativeMinter is ERC20, AllowList { | ||
// Precompiled Native Minter Contract Address | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we move the comments to the place of the new declarations? |
||
address constant MINTER_ADDRESS = 0x0200000000000000000000000000000000000001; | ||
// Designated Blackhole Address | ||
address constant BLACKHOLE_ADDRESS = 0x0100000000000000000000000000000000000000; | ||
string private constant TOKEN_NAME = "ERC20NativeMinterToken"; | ||
string private constant TOKEN_SYMBOL = "XMPL"; | ||
|
||
|
@@ -56,3 +57,4 @@ contract ERC20NativeMinter is ERC20, AllowList { | |
return 18; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,11 @@ pragma solidity ^0.8.0; | |
import "./IRewardManager.sol"; | ||
import "@openzeppelin/contracts/access/Ownable.sol"; | ||
|
||
address constant REWARD_MANAGER_ADDRESS = 0x0200000000000000000000000000000000000004; | ||
address constant BLACKHOLE_ADDRESS = 0x0100000000000000000000000000000000000000; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we keep the consts only used in tests in the test file? |
||
|
||
// ExampleRewardManager is a sample wrapper contract for RewardManager precompile. | ||
contract ExampleRewardManager is Ownable { | ||
address constant REWARD_MANAGER_ADDRESS = 0x0200000000000000000000000000000000000004; | ||
IRewardManager rewardManager = IRewardManager(REWARD_MANAGER_ADDRESS); | ||
|
||
constructor() Ownable() {} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,21 @@ | ||
//SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.0; | ||
|
||
import "@openzeppelin/contracts/access/Ownable.sol"; | ||
import "./AllowList.sol"; | ||
import "./IAllowList.sol"; | ||
|
||
// Precompiled Allow List Contract Address | ||
address constant TX_ALLOW_LIST = 0x0200000000000000000000000000000000000002; | ||
address constant OTHER_ADDRESS = 0x0Fa8EA536Be85F32724D57A37758761B86416123; | ||
|
||
// ExampleTxAllowList shows how TxAllowList precompile can be used in a smart contract | ||
// All methods of [allowList] can be directly called. There are example calls as tasks in hardhat.config.ts file. | ||
contract ExampleTxAllowList is AllowList { | ||
// Precompiled Allow List Contract Address | ||
address constant TX_ALLOW_LIST = 0x0200000000000000000000000000000000000002; | ||
|
||
constructor() AllowList(TX_ALLOW_LIST) {} | ||
|
||
function deployContract() public { | ||
new Example(); | ||
} | ||
} | ||
|
||
contract Example {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
//SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.0; | ||
|
||
import "../AllowList.sol"; | ||
import "ds-test/src/test.sol"; | ||
|
||
contract AllowListTest is DSTest { | ||
function assertRole(uint result, AllowList.Role role) internal { | ||
assertEq(result, uint(role)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,145 @@ | ||
//SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.0; | ||
|
||
import "../ERC20NativeMinter.sol"; | ||
import "../INativeMinter.sol"; | ||
import "./AllowListTest.sol"; | ||
|
||
// TODO: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for stripping this down to only the testing logic |
||
// this contract adds another (unwanted) layer of indirection | ||
// but it's the easiest way to match the previous HardHat testing functionality. | ||
// Once we completely migrate to DS-test, we can simplify this set of tests. | ||
contract Minter { | ||
ERC20NativeMinter token; | ||
|
||
constructor(address tokenAddress) { | ||
token = ERC20NativeMinter(tokenAddress); | ||
} | ||
|
||
function mintdraw(uint amount) external { | ||
token.mintdraw(amount); | ||
} | ||
|
||
function deposit(uint value) external { | ||
token.deposit{value: value}(); | ||
} | ||
} | ||
|
||
contract ERC20NativeMinterTest is AllowListTest { | ||
INativeMinter nativeMinter = INativeMinter(MINTER_ADDRESS); | ||
|
||
function setUp() public { | ||
// noop | ||
} | ||
|
||
function step_mintdrawFailure() public { | ||
ERC20NativeMinter token = new ERC20NativeMinter(1000); | ||
address tokenAddress = address(token); | ||
|
||
assertRole(nativeMinter.readAllowList(tokenAddress), AllowList.Role.None); | ||
|
||
try token.mintdraw(100) { | ||
assertTrue(false, "mintdraw should fail"); | ||
} catch {} // TODO should match on an error to make sure that this is failing in the way that's expected | ||
} | ||
|
||
function step_addMinter() public { | ||
ERC20NativeMinter token = new ERC20NativeMinter(1000); | ||
address tokenAddress = address(token); | ||
|
||
|
||
assertRole(nativeMinter.readAllowList(tokenAddress), AllowList.Role.None); | ||
|
||
nativeMinter.setEnabled(tokenAddress); | ||
|
||
assertRole(nativeMinter.readAllowList(tokenAddress), AllowList.Role.Enabled); | ||
} | ||
|
||
function step_adminMintdraw() public { | ||
ERC20NativeMinter token = new ERC20NativeMinter(1000); | ||
address tokenAddress = address(token); | ||
|
||
address testAddress = address(this); | ||
|
||
nativeMinter.setEnabled(tokenAddress); | ||
|
||
uint initialTokenBalance = token.balanceOf(testAddress); | ||
uint initialNativeBalance = testAddress.balance; | ||
|
||
uint amount = 100; | ||
|
||
token.mintdraw(amount); | ||
|
||
assertEq(token.balanceOf(testAddress), initialTokenBalance - amount); | ||
assertEq(testAddress.balance, initialNativeBalance + amount); | ||
} | ||
|
||
function step_minterMintdrawFailure() public { | ||
ERC20NativeMinter token = new ERC20NativeMinter(1000); | ||
address tokenAddress = address(token); | ||
|
||
Minter minter = new Minter(tokenAddress); | ||
address minterAddress = address(minter); | ||
|
||
nativeMinter.setEnabled(tokenAddress); | ||
|
||
uint initialTokenBalance = token.balanceOf(minterAddress); | ||
uint initialNativeBalance = minterAddress.balance; | ||
|
||
assertRole(initialTokenBalance, AllowList.Role.None); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this assertion be checking the Role against expected role and initial balance with 0? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow... good catch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I meant to be checking the balance. Changing this to |
||
|
||
try minter.mintdraw(100) { | ||
assertTrue(false, "mintdraw should fail"); | ||
} catch {} // TODO should match on an error to make sure that this is failing in the way that's expected | ||
|
||
assertEq(token.balanceOf(minterAddress), initialTokenBalance); | ||
assertEq(minterAddress.balance, initialNativeBalance); | ||
} | ||
|
||
function step_minterDeposit() public { | ||
ERC20NativeMinter token = new ERC20NativeMinter(1000); | ||
address tokenAddress = address(token); | ||
|
||
Minter minter = new Minter(tokenAddress); | ||
address minterAddress = address(minter); | ||
|
||
nativeMinter.setEnabled(tokenAddress); | ||
|
||
uint amount = 100; | ||
|
||
nativeMinter.mintNativeCoin(minterAddress, amount); | ||
|
||
uint initialTokenBalance = token.balanceOf(minterAddress); | ||
uint initialNativeBalance = minterAddress.balance; | ||
|
||
minter.deposit(amount); | ||
|
||
assertEq(token.balanceOf(minterAddress), initialTokenBalance + amount); | ||
assertEq(minterAddress.balance, initialNativeBalance - amount); | ||
} | ||
|
||
function step_mintdraw() public { | ||
ERC20NativeMinter token = new ERC20NativeMinter(1000); | ||
address tokenAddress = address(token); | ||
|
||
Minter minter = new Minter(tokenAddress); | ||
address minterAddress = address(minter); | ||
|
||
nativeMinter.setEnabled(tokenAddress); | ||
|
||
uint amount = 100; | ||
|
||
uint initialNativeBalance = minterAddress.balance; | ||
assertRole(initialNativeBalance, AllowList.Role.None); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above |
||
|
||
token.mint(minterAddress, amount); | ||
|
||
uint initialTokenBalance = token.balanceOf(minterAddress); | ||
assertEq(initialTokenBalance, amount); | ||
|
||
minter.mintdraw(amount); | ||
|
||
assertRole(token.balanceOf(minterAddress), AllowList.Role.None); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above |
||
assertEq(minterAddress.balance, amount); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove these in favor of the new enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do that in a new PR... Otherwise, this PR will never get over the line.
Originally, I didn't want some weird intermediate state with half JS tests and half Solidity tests, but in hindsight, that was a mistake.