Skip to content
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

Add test contract code for existing abi/wasm and clean up libraries/testing & unittests dependencies #612

Merged
merged 20 commits into from
Jan 26, 2023

Conversation

heifner
Copy link
Member

@heifner heifner commented Jan 3, 2023

Updated unittests/contracts/eosio.msig, unittests/contracts/eosio.system, unittests/contracts/eosio.token, unittests/contracts/eosio.wrap and libraries/testing/contracts/eosio.bios with contract code as close as possible to existing abi/wasm files. Before only the abi/wasm existed in the leap repo.

Rebuilt all contracts mentioned above with the latest CDT (3.2.0-dev ea6d926c3d2e9fa37dd513b8a946d8283b1aa233) and replaced the abi/wasm with the newly generated abi/wasm. Now all these contracts can be built using -DEOSIO_COMPILE_TEST_CONTRACTS=true.

libraries/testing no longer depends on unittests.
Moved snapshot_suites.hpp out of libraries/testing and back into unittests where it belongs.

Resolves #54

heifner added 14 commits January 3, 2023 11:54
…ove dependency of libraries/testing on unittests.
…its protocol feature has not been activated.
…ate contract code for eosio.bios and eosio.token to match as close as possible what matches the existing abi/wasm.
@heifner heifner changed the title WIP: Clean up libraries/testing & unittests dependencies Add test contract code for existing abi/wasm and clean up libraries/testing & unittests dependencies Jan 4, 2023
@heifner heifner marked this pull request as ready for review January 4, 2023 22:09
@spoonincode
Copy link
Member

Note: eosio.token is embedded, like eosio.bios, in eosio_testing_contracts library

hm, fixes eosnetworkfoundation/mandel#681 I think

@spoonincode
Copy link
Member

The libtester created here seems to have a problem building contract unit tests
https://github.com/eosnetworkfoundation/eos-system-contracts/actions/runs/3843842909

@heifner
Copy link
Member Author

heifner commented Jan 5, 2023

The libtester created here seems to have a problem building contract unit tests https://github.com/eosnetworkfoundation/eos-system-contracts/actions/runs/3843842909

This is probably because of these changes:
https://github.com/AntelopeIO/leap/pull/612/files#diff-3fe8325df2bcbadd6d0031003362dac51ecb2035e5342e871de92ba5f0123501L4
I'm not sure why that would cause an issue as it should be linked into eosio_testing. At this point I think we should just remove txn_test_gen_plugin and revert this section of the CMakeList.txt changes. Also move eosio.token back down into unittests where it belongs.

@heifner heifner added the OCI Work exclusive to OCI team label Jan 5, 2023
@spoonincode
Copy link
Member

I haven't really looked very closely, but is it possible the new static library eosio_testing_contracts needs to be part of EosioTester.cmake (both of them)? In this area

find_library(libtester eosio_testing @CMAKE_INSTALL_FULL_LIBDIR@ NO_DEFAULT_PATH)
find_library(libchain eosio_chain @CMAKE_INSTALL_FULL_LIBDIR@ NO_DEFAULT_PATH)
find_library(libfc fc @CMAKE_INSTALL_FULL_LIBDIR@ NO_DEFAULT_PATH)
find_library(libsecp256k1 secp256k1 @CMAKE_INSTALL_FULL_LIBDIR@ NO_DEFAULT_PATH)
find_library(libbn256 bn256 @CMAKE_INSTALL_FULL_LIBDIR@ NO_DEFAULT_PATH)

Also that means eosio_testing_contracts would need to be cmake install()ed

@heifner
Copy link
Member Author

heifner commented Jan 5, 2023

but is it possible the new static library eosio_testing_contracts needs to be part of EosioTester.cmake (both of them)

I was assuming it was something like that. I really think the best approach is to go back to eosio_testing_contracts being an INTERFACE and not a library. That all can be put back like it was once txn_test_gen_plugin is removed.

…ove back into ./unittests/contracts and ./libraries/testing/CMakeLists.txt can go back to eosio_testing_contracts being INTERFACE only.
… the bios contract used in eos-system-contracts avoiding a duplicate trx error on set_code of its bios contract.
@heifner
Copy link
Member Author

heifner commented Jan 13, 2023

@heifner heifner requested a review from spoonincode January 13, 2023 16:40
@spoonincode
Copy link
Member

with contract code as close as possible to existing abi/wasm files

I had a hard time finding where this contract source originated from. Especially unittests/contracts/eosio.system doesn't seem to match anything I can find in reference-contracts. How sensitive are the tests to using the latest reference contracts? Is it possible (or silly) to use the reference contracts as a submodule?

@heifner
Copy link
Member Author

heifner commented Jan 13, 2023

with contract code as close as possible to existing abi/wasm files

I had a hard time finding where this contract source originated from. Especially unittests/contracts/eosio.system doesn't seem to match anything I can find in reference-contracts. How sensitive are the tests to using the latest reference contracts? Is it possible (or silly) to use the reference contracts as a submodule?

I had a hard time finding the source for these contracts. I dug around in github history, but for some of them the contract code corresponding to the time of the wasm change does not match the wasm. Some of it was trial and error to get to something that works.

I think ideally we would not use any reference-contracts in our tests. Something like them are needed to test protocol features and other host functions. We want the contracts to remain static for our tests. This is likely why we didn't before bother including the source code for them in leap. However, I do believe we should be able to recreate the wasms from source if desired, hence this PR.

Copy link
Member

@spoonincode spoonincode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a follow up question on whether it made sense (not now in this PR, but in the future) to have a CI job ensure the contracts are still compiling 1:1 with checked in binaries. I feel like you may be indirectly answered that already though..

@heifner
Copy link
Member Author

heifner commented Jan 13, 2023

I had a follow up question on whether it made sense (not now in this PR, but in the future) to have a CI job ensure the contracts are still compiling 1:1 with checked in binaries. I feel like you may be indirectly answered that already though..

Ideally it would be nice to have a job that makes sure the contracts still compile with the latest CDT and all tests pass with the latest CDT compiled contracts.

@heifner heifner requested a review from larryk85 January 18, 2023 22:00
@heifner heifner merged commit cab88df into main Jan 26, 2023
@heifner heifner deleted the GH-54-contracts branch January 26, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test Contracts Source Files
3 participants