Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Use staticcall for external function calls in MixinSignatureValidator #1012

Merged
merged 5 commits into from Aug 24, 2018

Conversation

abandeali1
Copy link
Member

Description

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Prefix PR title with bracketed package name(s) corresponding to the changed package(s). For example: [sol-cov] Fixed bug.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@coveralls
Copy link

coveralls commented Aug 24, 2018

Coverage Status

Coverage remained the same at 85.903% when pulling 6a9669a on feature/contracts/staticcall into 7f36574 on development.

@abandeali1 abandeali1 force-pushed the feature/contracts/staticcall branch 2 times, most recently from f4c5a77 to 6c72d4f Compare August 24, 2018 16:32
Copy link
Contributor

@recmo recmo left a comment

Choose a reason for hiding this comment

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

I prefer this approach over pragma experimental "v0.5.0"!

32 // output size is 32 bytes
)
// Signature is valid if call did not revert and returned true
isValid := and(success, mload(cdStart))
Copy link
Contributor

Choose a reason for hiding this comment

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

This only works if the called contract returns a non-zero odd number for true, like 1. I'm not sure if the ABI guarantees that boolean true is always encoded as 1 (instead of anything non-zero like in C).

To fix we could do and(success, iszero(iszero(mload(cdStart)))).

32 // output size is 32 bytes
)
// Signature is valid if call did not revert and returned true
isValid := and(success, mload(cdStart))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

view
returns (bool isValid)
{
bytes memory calldata = abi.encodeWithSelector(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will bumb the memory pointer unnecessarily, leading to extra gas cost. (not an issue)

view
returns (bool isValid)
{
bytes memory calldata = abi.encodeWithSelector(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

@abandeali1 abandeali1 merged commit 7351bf0 into development Aug 24, 2018
@abandeali1 abandeali1 deleted the feature/contracts/staticcall branch August 28, 2018 05:02
walletAddress, // address of Wallet contract
cdStart, // pointer to start of input
mload(calldata), // length of input
cdStart, // write input over output

Choose a reason for hiding this comment

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

Should be
// write output over input

validatorAddress, // address of Validator contract
cdStart, // pointer to start of input
mload(calldata), // length of input
cdStart, // write input over output

Choose a reason for hiding this comment

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

Should be
// write output over input

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.

None yet

4 participants