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: extract proxy factory from gnosis safe and test it #121

Merged
merged 8 commits into from
Oct 2, 2021

Conversation

DavidMinarsch
Copy link
Contributor

Proposed changes

This PR extracts each contract into its own package. Also adds tests for the extracted functionality.

Fixes

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING doc
  • I am making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

Further comments

Comment on lines +228 to 229
) = GnosisSafeProxyFactoryContract.build_tx_deploy_proxy_contract_with_nonce(
ledger_api,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

contracts packages can depend on others. The framework doesn't currently account for this. We need to add an issue on aea repo and support this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return safe_contract

@classmethod
def _build_tx_deploy_proxy_contract_with_nonce( # pylint: disable=too-many-arguments
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted to the contract gnosis_safe_proxy_factory

f"aea.packages.{PUBLIC_ID.author}.contracts.{PUBLIC_ID.name}.contract"
)

PROXY_FACTORY_CONTRACT = "0xa6B71E26C5e0845f74c812102Ca7114b6a896AB2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the address IFF the contract is deployed using deterministic deployment (https://ethereum-magicians.org/t/deterministic-deployment-proxy-magic-wrapped-in-magic/3261/7). At the moment I am not sure how to do this on plain ganache.

Comment on lines 50 to 52
def setup(
self,
):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to run this as a classmethod (setup_class(cls)). But UseGanache doesn't work then. @marcofavorito how can we refactor UseGanache to support both setup and setup_class?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes we can, preparing a PR to show my proposal

from tests.helpers.docker.ganache import DEFAULT_GANACHE_PORT


class BaseContractTest(UseGanache):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once the below issue is addressed, this can be extracted as a generic utility class for contract testing.

@codecov
Copy link

codecov bot commented Oct 2, 2021

Codecov Report

Merging #121 (7ca0a2d) into main (ce682c8) will increase coverage by 0.07%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
+ Coverage   88.66%   88.73%   +0.07%     
==========================================
  Files          60       62       +2     
  Lines        5424     5442      +18     
==========================================
+ Hits         4809     4829      +20     
+ Misses        615      613       -2     
Flag Coverage Δ
unittests 88.73% <95.83%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/valory/contracts/gnosis_safe/contract.py 69.84% <77.77%> (-2.77%) ⬇️
packages/valory/contracts/gnosis_safe/__init__.py 100.00% <100.00%> (ø)
...ry/contracts/gnosis_safe_proxy_factory/__init__.py 100.00% <100.00%> (ø)
...ry/contracts/gnosis_safe_proxy_factory/contract.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce682c8...7ca0a2d. Read the comment docs.

@DavidMinarsch DavidMinarsch merged commit e457435 into main Oct 2, 2021
@DavidMinarsch DavidMinarsch deleted the feat/refactor-contracts branch October 8, 2021 20:03
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