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

Detect MultiSend addresses #299

Merged
merged 1 commit into from
Jul 12, 2022
Merged

Detect MultiSend addresses #299

merged 1 commit into from
Jul 12, 2022

Conversation

@coveralls
Copy link

coveralls commented Jul 11, 2022

Pull Request Test Coverage Report for Build 2656170144

  • 17 of 18 (94.44%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 91.068%

Changes Missing Coverage Covered Lines Changed/Added Lines %
gnosis/eth/ethereum_client.py 0 1 0.0%
Totals Coverage Status
Change from base Build 2656053853: 0.03%
Covered Lines: 2967
Relevant Lines: 3258

💛 - Coveralls

Copy link

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

Looks like it does the trick! Would be really cool if the build_tx_data was reachable without a real NODE_URL.

I suppose if the contract ABI were exported I could just encode the function call with a dummy web3 on my own.

"%s proxy factory address not valid" % address
)
def __init__(
self, ethereum_client: EthereumClient, address: Optional[ChecksumAddress] = None

Choose a reason for hiding this comment

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

I know its probably too much to ask, but you think we can also pass along a Dummy EthereumClient here? I am just hoping for some way of encoding the function call build_tx_data(transactions).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Comment on lines +182 to +185
assert fast_is_checksum_address(address), (
"%s proxy factory address not valid" % address
)

Choose a reason for hiding this comment

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

Probably off topic, but is there any reason you don't enforce that the address input is a ChecksumAddress?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm already checking if it's a ChecksumAddress

MULTISEND_ADDRESSES = (
"0xA238CBeb142c10Ef7Ad8442C6D1f9E89e07e7761",
"0x998739BFdAAdde7C933B942a68053933098f9EDa",
)
Copy link
Member

Choose a reason for hiding this comment

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

There is any reason to use Tuple instead List?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not very relevant, but Tuple is inmutable and in this case we don't plan on modifying it

gnosis/safe/tests/test_multi_send.py Show resolved Hide resolved
@Uxio0 Uxio0 merged commit 8848f16 into master Jul 12, 2022
@Uxio0 Uxio0 deleted the detect-multisend-address branch July 12, 2022 13:12
@github-actions github-actions bot locked and limited conversation to collaborators Jul 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Multisend constructor address field Optional
4 participants