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

Feature/909 proxy delegatecall #1003

Merged
merged 34 commits into from
Sep 30, 2019
Merged

Feature/909 proxy delegatecall #1003

merged 34 commits into from
Sep 30, 2019

Conversation

r00ted199
Copy link
Contributor

@r00ted199 r00ted199 commented Sep 15, 2019

Description

The intention of this PR is to validate if an address is a contract or EOA before calling the delegatecall

Tested

Test added in proxy.ts calling _setAndInitializeImplementation with a EOA address.

Other changes

The isContract function shouldn't be called from a constructor. Calling this function from a constructor will return 0 independently if the address given as parameter is a contract or EOA

Related issues

Backwards compatibility

Brief explanation of why these changes are/are not backwards compatible.

@codecov
Copy link

codecov bot commented Sep 15, 2019

Codecov Report

Merging #1003 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1003   +/-   ##
=======================================
  Coverage   66.59%   66.59%           
=======================================
  Files         257      257           
  Lines        7394     7394           
  Branches      494      430   -64     
=======================================
  Hits         4924     4924           
- Misses       2373     2375    +2     
+ Partials       97       95    -2
Flag Coverage Δ
#mobile 66.59% <ø> (ø) ⬆️
Impacted Files Coverage Δ
packages/mobile/src/utils/formatting.ts 87.5% <0%> (ø) ⬆️
packages/mobile/src/identity/reducer.ts 41.66% <0%> (ø) ⬆️

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 2b39844...f5f63bb. Read the comment docs.

.circleci/config.yml Outdated Show resolved Hide resolved
packages/protocol/contracts/common/Proxy.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/common/Proxy.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/common/Proxy.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/common/Proxy.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/common/Proxy.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/common/Proxy.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/common/Proxy.sol Outdated Show resolved Hide resolved
@m-chrzan
Copy link
Contributor

Transfer tests are failing. They are coupled to gas costs of various smart contract calls. Changing Proxy's fallback function will have increased gas costs for pretty much every call. This will unfortunately require changes to the e2e transfer test (updating the expected gas costs) and to params/protocol_params.go in celo-blockchain, to pass enough gas to the EVM when making internal EVM calls.

You can run the tests locally by going to the celotool package and running

./ci_test_transfers.sh local <path to local celo-blockchain repo>

Let me know if you have any questions, happy to chat about this.

@aaitor aaitor added the automerge Have PR merge automatically when checks pass label Sep 18, 2019
@aaitor aaitor assigned aaitor and unassigned r00ted199 Sep 18, 2019
@r00ted199 r00ted199 requested a review from m-chrzan September 20, 2019 09:19
packages/protocol/contracts/governance/Governance.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/governance/Governance.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/common/MultiSig.sol Outdated Show resolved Hide resolved
@asaj
Copy link
Contributor

asaj commented Sep 23, 2019

@asaj, @m-chrzan , this PR is pending of some broken tests in master. Let me know if is good to merge or you prefer till those changes are fixed there first.

I think it's worth waiting to fix these, I think the fix should be quick and simple

@asaj asaj unassigned asaj and m-chrzan Sep 23, 2019
@aaitor aaitor requested a review from asaj September 24, 2019 12:47
Bringing celo-monorepo:master
@aaitor aaitor added the automerge Have PR merge automatically when checks pass label Sep 25, 2019
@m-chrzan m-chrzan added automerge Have PR merge automatically when checks pass and removed automerge Have PR merge automatically when checks pass labels Sep 30, 2019
@ashishb ashishb merged commit ecc3d1b into celo-org:master Sep 30, 2019
cmcewen added a commit that referenced this pull request Sep 30, 2019
aaronmgdr added a commit that referenced this pull request Oct 1, 2019
* master: (31 commits)
  Upgrade to Node 10 (#1148)
  [faucet] Add custom metrics (#1143)
  Add IdentityMetadata to Contractkit (#1126)
  [Wallet] Local currency v1.1 (#1137)
  Add attestation-service deploy (#1128)
  [Wallet] A few docs and build cleanup (#1138)
  [CircleCI]Add comment on how to fix lint checks (#1134)
  2019-09-30 integration deployment (#1149)
  Update web3 provider to new integration url (#1151)
  [celotool]Add fast mode to celotool invite (#1135)
  Revert "Feature/909 proxy delegatecall" (#1146)
  Use contractkit in notification service (#1118)
  Feature/909 proxy delegatecall (#1003)
  integration deployment for 2019-09-29 (#1139)
  Add instructions for npm publication to tag commit (#1117)
  Client Logs Data Flow script update (#1055)
  Deploying latest proxy code in genesis (#1122)
  Enable floating promises check everywhere (fix issues) (#1115)
  [cli] Solution for build error contractkit on Linux 19.04 distro (#960)
  [Wallet] Merge back changes made for mx pilot (#1113)
  ...

# Conflicts:
#	yarn.lock
aaronmgdr added a commit that referenced this pull request Oct 1, 2019
* master: (35 commits)
  [Wallet] Network fee in transaction feed (#1145)
  New About Page Cover (#905)
  Upgrade to Node 10 (#1148)
  [faucet] Add custom metrics (#1143)
  Add IdentityMetadata to Contractkit (#1126)
  [Wallet] Local currency v1.1 (#1137)
  Add attestation-service deploy (#1128)
  [Wallet] A few docs and build cleanup (#1138)
  [CircleCI]Add comment on how to fix lint checks (#1134)
  2019-09-30 integration deployment (#1149)
  Update web3 provider to new integration url (#1151)
  [celotool]Add fast mode to celotool invite (#1135)
  Revert "Feature/909 proxy delegatecall" (#1146)
  Use contractkit in notification service (#1118)
  Feature/909 proxy delegatecall (#1003)
  integration deployment for 2019-09-29 (#1139)
  Add instructions for npm publication to tag commit (#1117)
  Client Logs Data Flow script update (#1055)
  Deploying latest proxy code in genesis (#1122)
  Enable floating promises check everywhere (fix issues) (#1115)
  ...

# Conflicts:
#	packages/web/src/about/About.tsx
#	packages/web/src/about/images/index.ts
#	packages/web/static/locales/en/about.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Developers SBAT check contract code existence in Proxy contract
6 participants