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

Revert "Feature #909 proxy delegatecall" #1241

Merged
merged 1 commit into from
Oct 8, 2019

Conversation

asaj
Copy link
Contributor

@asaj asaj commented Oct 8, 2019

Reverts #1152

This seems to have broken the end-to-end tests, which due to a yet-to-be-found bug in the incremental testing code, did not run when this PR was submitted.

The specific issue arises when trying to submit transactions before the registry implementation is deployed.

GetRegisteredAddressWithEvm returns evm: execution reverted rather than errors.ErrRegistryContractNotDeployed or errors.ErrSmartContractNotDeployed, because there is no code at the implementation address, which, with the changes in this PR, cause the call to fail.

https://github.com/celo-org/celo-blockchain/blob/master/core/vm/dynamic_params.go#L46

This causes us to fail to fetch the gas price minimum in tx_pool.go, and we reject the transaction.

https://github.com/celo-org/celo-blockchain/blob/master/core/tx_pool.go#L674

I recommend we first find and fix the bug in incremental testing, and then resubmit this PR along with an accompanying celo-blockchain PR that returns a different error when the call in GetRegisteredAddressWithEvm reverts, or possibly use a different proxy for the registry that doesn't include this check.

@asaj asaj requested a review from m-chrzan as a code owner October 8, 2019 02:53
@asaj asaj assigned asaj and unassigned aaitor Oct 8, 2019
@codecov
Copy link

codecov bot commented Oct 8, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1241   +/-   ##
=======================================
  Coverage   66.47%   66.47%           
=======================================
  Files         262      262           
  Lines        7648     7648           
  Branches      441      441           
=======================================
  Hits         5084     5084           
  Misses       2464     2464           
  Partials      100      100
Flag Coverage Δ
#mobile 66.47% <ø> (ø) ⬆️

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 9a50adf...b491453. Read the comment docs.

@asaj
Copy link
Contributor Author

asaj commented Oct 8, 2019

Upon second glance, it seems like we would still have the same registry proxy code, since it's hard coded into the genesis block, which this PR didn't change. So I'm not totally sure why this seems to be causing issues.

If anything, I now would have thought that celo-org/celo-blockchain#507 would cause the issue. The story is a bit muddled by the end-to-end tests not being pointed to master during some of this time.

Still suggest we revert to get the end-to-end tests passing again.

@asaj asaj merged commit d05e4c5 into master Oct 8, 2019
aaronmgdr added a commit that referenced this pull request Oct 8, 2019
* master: (35 commits)
  [Wallet] Fix top of emojis cut off in the activity feed (#1243)
  Adding a contract to store minimum required client version (#1081)
  Revert "Feature #909 proxy delegatecall (#1152)" (#1241)
  Use ContractKit to get addresses for Blockchain API (#1175)
  Feature #909 proxy delegatecall (#1152)
  Fix Faucet done message (#1217)
  Updated SETUP.md with new yarn process (#1224)
  Adding `increaseAllowance` and `decreaseAllowance` methods (#1196)
  extracting function signatures (#1061)
  Fix integration hardcode (#1208)
  Fixing flaky governance test (#1155)
  Restore CI branch (#1223)
  [wallet] e2e back to green (#1210)
  [Wallet] Implement new import wallet flow designs (#1209)
  [Wallet] Fix disable conditions for butons on Enter Invite screen (#1214)
  [protocol] Rename infrastructureFraction to proposerFraction (#1174)
  [ck] Proper promise treatment to avoid UnhandledPromises (#1219)
  [ck] Transform StableToken parameters from fixidity format (#1218)
  [wallet]Store encrypted local signing key (#1188)
  2019-10-03 alfajores deployment (#1200)
  ...
@mrsmkl
Copy link
Contributor

mrsmkl commented Oct 9, 2019

Seems like the problem is that at some point, the proxy contract is called before the implementationAddress has been set. After #1122, up-to-date proxy code is loaded into the genesis block when running e2e tests. (If you test locally, note that you have to build the contracts before running the test)

@mcortesi mcortesi deleted the revert-1152-feature/909-proxy-delegatecall branch February 14, 2020 22:02
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.

4 participants