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

refactor: improve contract test framework #3000

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

darwintree
Copy link
Contributor

@darwintree darwintree commented Dec 13, 2024

This pull request refactored the ConfluxTestFrameworkForContract in tests/test_framework/contracts.py and refactored the relating tests according to the new contract test framework.

The rewrite use the core space SDK to replace the legacy implementation, making it easier for:

  • transaction build and sending
  • genesis account setup
  • address format management
  • better type hints support
  • universal interface with web3.py and easy setup for future espace tests

This change is Reviewable

Copy link
Contributor

@ChenxingLi ChenxingLi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 17 files reviewed, 1 unresolved discussion (waiting on @darwintree)


tests/admin_at_creation_test.py line 8 at r1 (raw file):

from web3 import Web3

class ClearAdminTest(ConfluxTestFrameworkForContract):

Do we still need ConfluxTestFrameworkForContract?

Btw, some tests may rely on the test param executive_trace = "true", which is enabled by ConfluxTestFrameworkForContract by default. These tests may fail after removing ConfluxTestFrameworkForContract.

@darwintree
Copy link
Contributor Author

darwintree commented Dec 19, 2024

Reviewable status: 0 of 17 files reviewed, 1 unresolved discussion (waiting on @darwintree)

tests/admin_at_creation_test.py line 8 at r1 (raw file):

from web3 import Web3

class ClearAdminTest(ConfluxTestFrameworkForContract):

Do we still need ConfluxTestFrameworkForContract?

Btw, some tests may rely on the test param executive_trace = "true", which is enabled by ConfluxTestFrameworkForContract by default. These tests may fail after removing ConfluxTestFrameworkForContract.

I think it's ok. There may be some side effect on setup(for example, auto block generation on startup), but they can be easily avoided by rewriting before_test. I will merge the current version ConfluxTestFrameworkForContract and base class to see if tests run well

Copy link
Contributor

@ChenxingLi ChenxingLi left a comment

Choose a reason for hiding this comment

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

Reviewed 14 of 17 files at r1, all commit messages.
Reviewable status: 14 of 17 files reviewed, 3 unresolved discussions (waiting on @darwintree)


tests/admin_at_creation_test.py line 8 at r1 (raw file):

Previously, darwintree wrote…

I think it's ok. There may be some side effect on setup(for example, auto block generation on startup), but they can be easily avoided by rewriting before_test. I will merge the current version ConfluxTestFrameworkForContract and base class to see if tests run well

OK, I think we can keep it and remove the unnecessary toolkits. The current update is ok.


tests/commission_privilege_test.py line 114 at r1 (raw file):

        test_contract.functions.remove(addr4).transact({
            "storageLimit": 0,
            "gas": gas,

Can we avoid repeating "gas": gas and "gasPrice": 1?


tests/contract_remove_test.py line 88 at r1 (raw file):

        storage_contract = self.deploy_contract_and_init(5)
        multi_calldata = [
            storage_contract.functions.change(3)._encode_transaction_data(),

It is counter-intuitive to use a private Python function to compute calldata.


tests/overlay_account_storage_test.py line 134 at r1 (raw file):

        self.internal_contract("SponsorWhitelistControl").functions.setSponsorForCollateral(storage_contract.address).transact({
            "value": 1000 * 10 ** 18

Is there a utility function in the Web3 SDK that makes multiplying by 10 ** 18 more semantic?

Copy link
Contributor Author

@darwintree darwintree left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 26 files reviewed, 3 unresolved discussions (waiting on @ChenxingLi)


tests/admin_at_creation_test.py line 8 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

OK, I think we can keep it and remove the unnecessary toolkits. The current update is ok.

I tried removing the class and it seems current version works now


tests/commission_privilege_test.py line 114 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

Can we avoid repeating "gas": gas and "gasPrice": 1?

The question is that this test design the gas cost very delicately, so I tried to replay the legacy code, or errors might happen. In normal cases, if we don't care gas cost, then we don't need to specify them every time.


tests/contract_remove_test.py line 88 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

It is counter-intuitive to use a private Python function to compute calldata.

done


tests/overlay_account_storage_test.py line 134 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

Is there a utility function in the Web3 SDK that makes multiplying by 10 ** 18 more semantic?

There are 2 ways.

  1. web3.py implements w3.to_wei(10, "ether")
  2. conflux-web3 implements a token unit class, which enables easy and safe token unit calculations (for example, direct CFX and Drip calculation)

Several use cases are currently replaced with the second approach

@darwintree
Copy link
Contributor Author

retest this please

1 similar comment
@darwintree
Copy link
Contributor Author

retest this please

@darwintree darwintree closed this Dec 23, 2024
@darwintree darwintree reopened this Dec 23, 2024
Copy link
Contributor

@ChenxingLi ChenxingLi left a comment

Choose a reason for hiding this comment

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

Reviewed 25 of 26 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @darwintree)


tests/cip107_test.py line 153 at r3 (raw file):

    def run_test(self):
        self.w3 = self.cw3
        self.sponsorControl = self.w3.cfx.contract(name="SponsorWhitelistControl", with_deployment_info=True)

Why not self.internal_contract(name="SponsorWhitelistControl")?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants