-
Notifications
You must be signed in to change notification settings - Fork 278
feat: convert native minter npm test to go test #1872
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
base: master
Are you sure you want to change the base?
feat: convert native minter npm test to go test #1872
Conversation
a93e2b6 to
b7eec85
Compare
4131d69 to
ef32f2f
Compare
| @@ -0,0 +1,13 @@ | |||
| // Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved. | |||
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.
This whole package seems like a mess. Is the correct structure? It's not like the deployerallowlist package, because the minter is not tested seperately like allowlist is.
| @@ -1,7 +1,7 @@ | |||
| // Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved. | |||
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 moved these in to match what you did for the deployerallowlist. I think all of these folders need to be cleaned up / reorganized, but I would appreciate your guidance @ceyonur
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.
xxxtest packages are mostly meant for test-related but not actual UT files (test utility files mostly). There is a cyclic dependency so we cannot really put these simulatedtest/binding files under the related precompile packages (like nativeminter/). I was going to suggest reverting other changes and add another simulatedtest packages but I was not aware that we had another cyclic dependency introduced with extstate cleanup.
I think in that case we can move all these precompile tests to their relevant test packages. (like nativemintertest or testnativeminter etc). If you feel like it will be cleaner we can introduce simulatetest packages, but it seems that we need to move config_test and contract_test into xxxtest packages anyway.
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.
trying to put everything in the top level package also results in cyclic dependencies.
Would you like to put a PR up into this PR to restructure the files? I'm not sure if I follow exactly what you are suggesting. It really is a mess...
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.
Yeah gosh this is really messy. What are your thoughts on another bindings folder? or some way to split things up better?
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.
see #1878
| test func(t *testing.T, backend *sim.Backend, nativeMinterIntf *INativeMinter) | ||
| } | ||
|
|
||
| testCases := []testCase{ |
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.
all of these tests are 1:1 copies of the tests in https://github.com/ava-labs/subnet-evm/blob/c65752b631b07206d506111cd52e5d686a9f02b0/contracts/contracts/test/ERC20NativeMinterTest.sol
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.
We should be getting rid of these ERC20 related testing (and openzeppelin lib)
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.
Is the coverage still good now? post removal?
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.
yea I think so.
| @@ -1,27 +1,31 @@ | |||
| //SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.24; | |||
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.
can we get rid of this file? I don't think this is a viable testing for this and we should not be importing any extra contracts.
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.
Instead we can use a much simpler approach that only issues mintNativeCoin (similar to allowlist tests.)
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.
Yup I've deleted this file entirely -- let me know what you think of NativeMinterTest.
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.
it looks good!
| @@ -1,7 +1,7 @@ | |||
| // Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved. | |||
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.
xxxtest packages are mostly meant for test-related but not actual UT files (test utility files mostly). There is a cyclic dependency so we cannot really put these simulatedtest/binding files under the related precompile packages (like nativeminter/). I was going to suggest reverting other changes and add another simulatedtest packages but I was not aware that we had another cyclic dependency introduced with extstate cleanup.
I think in that case we can move all these precompile tests to their relevant test packages. (like nativemintertest or testnativeminter etc). If you feel like it will be cleaner we can introduce simulatetest packages, but it seems that we need to move config_test and contract_test into xxxtest packages anyway.
| @@ -1,7 +1,7 @@ | |||
| //SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.24; | |||
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.
why did we move these files to Solidity? I think it makes sense to keep them together with their bindings.
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 was just trying to simplify the structure -- I think it needs a more thorough approach though.
| test func(t *testing.T, backend *sim.Backend, nativeMinterIntf *INativeMinter) | ||
| } | ||
|
|
||
| testCases := []testCase{ |
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.
We should be getting rid of these ERC20 related testing (and openzeppelin lib)
| func setAsEnabled(t *testing.T, b *sim.Backend, nativeMinter *INativeMinter, auth *bind.TransactOpts, address common.Address) { | ||
| t.Helper() | ||
| tx, err := nativeMinter.SetEnabled(auth, address) | ||
| require.NoError(t, err) | ||
| testutils.WaitReceiptSuccessful(t, b, tx) | ||
| } |
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 this can be move to allowlisttest package as a test helper.
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 can't move a test package there because there's another importcycle when I do it that way
feemanager (test) -> allowlisttest -> ethclient/simulated -> core -> feemanager
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.
So I put the allowlist test helpers in testutils.
| return addr, contract | ||
| } | ||
|
|
||
| func verifyRole(t *testing.T, nativeMinter *INativeMinter, address common.Address, expectedRole allowlist.Role) { |
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.
This can be move to allowlisttest package as a test helper.
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.
See above comment.
precompile/contracts/nativeminter/nativemintertest/simulated_test.go
Outdated
Show resolved
Hide resolved
ceyonur
left a comment
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.
overall it's looking good. We just need to the helper file and decide the test structure in #1878
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/ava-labs/subnet-evm/accounts/abi/bind" | ||
| "github.com/ava-labs/subnet-evm/precompile/allowlist" |
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.
Isn't it best to move this file to the allowlisttest to reduce the imported packages?
| test func(t *testing.T, backend *sim.Backend, nativeMinterIntf *INativeMinter) | ||
| } | ||
|
|
||
| testCases := []testCase{ |
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.
yea I think so.
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.
the file looking good 👍 , it's just would fit better to allowlisttest.
| @@ -1,27 +1,31 @@ | |||
| //SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.24; | |||
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.
it looks good!
| @@ -1,7 +1,7 @@ | |||
| // Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved. | |||
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.
see #1878
Why this should be merged
Replaces native minter e2e hardhat tests with golang bindings and a simulated backend test.
Part of #1228
How this works
Generates relevant golang bindings from sol files for the testing. Mirrors the hardhat test to a golang test using simulated backend.
How this was tested
CI
Need to be documented?
No
Need to update RELEASES.md?
No