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

feat: add migration to upgrade contracts #54

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

fedealconada
Copy link
Contributor

upgrades:

  • KYC viewer
  • Paymaster
  • Wallet
  • Factory
  • Registry

@fedealconada fedealconada requested a review from rrecuero January 23, 2024 19:11
Copy link

Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary

Copy link

github-actions bot commented Jan 23, 2024

Changes to gas cost

Generated at commit: 0267906216472b5a63c15349123f2065f0e6a3c1, compared to commit: d858d95ccc91dad8db5e67dc3e0e08e687681ccd

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
KintoID burnKYC +1,989 ❌ +4.97%
KintoAppRegistry getAppMetadata -143 ✅ -4.57%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
KintoID 3,805,454 (0) burnKYC
hasRole
isKYC
mintIndividualKyc
nonces
3,390 (0)
699 (0)
739 (0)
1,740 (0)
631 (0)
0.00%
0.00%
0.00%
0.00%
0.00%
42,009 (+1,989)
2,299 (+267)
2,037 (-3)
231,755 (+145)
2,579 (+2)
+4.97%
+13.14%
-0.15%
+0.06%
+0.08%
60,533 (+3,350)
2,699 (0)
1,335 (0)
236,817 (0)
2,631 (0)
+5.86%
0.00%
0.00%
0.00%
0.00%
63,883 (0)
2,699 (0)
7,335 (0)
263,882 (0)
2,631 (0)
0.00%
0.00%
0.00%
0.00%
0.00%
12 (+1)
5 (+2)
558 (+9)
183 (+7)
195 (+8)
KintoAppRegistry 2,929,992 (0) getAppMetadata
getContractLimits
registerApp
updateMetadata
2,758 (0)
3,015 (0)
101,123 (-44,388)
7,805 (-448)
0.00%
0.00%
-30.50%
-5.43%
2,986 (-143)
3,779 (-1)
299,912 (+137)
35,237 (-49)
-4.57%
-0.03%
+0.05%
-0.14%
3,001 (-143)
3,184 (0)
307,815 (0)
32,253 (0)
-4.55%
0.00%
0.00%
0.00%
3,184 (-286)
19,184 (0)
326,767 (-44,388)
57,105 (0)
-8.24%
0.00%
-11.96%
0.00%
4 (0)
668 (0)
143 (+19)
9 (0)
SafeBeaconProxy 287,998 (0) owners
whitelistApp
1,815 (0)
13,912 (0)
0.00%
0.00%
7,724 (+165)
24,603 (+36)
+2.18%
+0.15%
1,815 (0)
24,812 (0)
0.00%
0.00%
14,815 (0)
24,812 (0)
0.00%
0.00%
44 (+1)
130 (+19)
EntryPoint 3,072,757 (0) depositTo
setWalletFactory
walletFactory
2,613 (0)
2,586 (0)
447 (0)
0.00%
0.00%
0.00%
13,324 (-33)
22,519 (+5)
762 (-12)
-0.25%
+0.02%
-1.55%
7,413 (0)
22,648 (0)
447 (0)
0.00%
0.00%
0.00%
24,513 (0)
22,648 (0)
2,447 (0)
0.00%
0.00%
0.00%
281 (+38)
156 (+6)
190 (+7)
Counter 43,099 (0) count 261 (0) 0.00% 276 (-2) -0.72% 261 (0) 0.00% 2,261 (0) 0.00% 131 (+19)
SponsorPaymaster 2,188,351 (0) addDepositFor 22,606 (-9,800) -30.24% 43,197 (-115) -0.27% 39,706 (-2,282) -5.43% 54,306 (-2,500) -4.40% 281 (+38)
UpgradeableBeacon 216,907 (0) upgradeTo 2,009 (0) 0.00% 2,035 (-1) -0.05% 2,009 (0) 0.00% 6,809 (0) 0.00% 182 (+6)
KintoWalletFactoryUpgrade 2,882,513 (+59,478)
KintoWalletUpgrade 2,850,294 (-15)
SponsorPaymasterExploitTest 31,315,277 (+58,023)

@fedealconada fedealconada force-pushed the fix-paymaster-and-wallet branch from 81c1b99 to d858d95 Compare January 23, 2024 19:15
KintoAppRegistryV3 _registryImpl = KintoAppRegistryV3(upgradeRegistry());
KintoWalletV4 _walletImpl = KintoWalletV4(payable(upgradeWallet()));
KintoWalletFactoryV7 _factoryImpl = KintoWalletFactoryV7(upgradeFactory());
KYCViewerV2 _kycViewerImpl = KYCViewerV2(upgradeKYCViewer());
Copy link
Contributor

Choose a reason for hiding this comment

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

need to redeploy kyc viewer completely. The previous one is not usable because we didnt call disableInitializers

console.log(string.concat("KintoAppRegistryV3-impl: ", vm.toString(address(_registryImpl))));
console.log(string.concat("KintoWalletV4-impl: ", vm.toString(address(_walletImpl))));
console.log(string.concat("KintoWalletFactoryV7-impl: ", vm.toString(address(_factoryImpl))));
console.log(string.concat("KYCViewerV2-impl: ", vm.toString(address(_kycViewerImpl))));
Copy link
Contributor

Choose a reason for hiding this comment

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

add 'KYCViewer' and rename that to KYCViewerV1-impl

bytes memory bytecode =
abi.encodePacked(type(KintoAppRegistryV3).creationCode, abi.encode(address(_walletFactory)));

vm.broadcast(deployerPrivateKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

transferOwnership to ledger before upgrade?

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d858d95) 82.02% compared to head (cceee1a) 82.24%.

Additional details and impacted files
@@                     Coverage Diff                      @@
##           fix-paymaster-and-wallet      #54      +/-   ##
============================================================
+ Coverage                     82.02%   82.24%   +0.22%     
============================================================
  Files                             8        8              
  Lines                           445      445              
  Branches                        123      123              
============================================================
+ Hits                            365      366       +1     
  Misses                           35       35              
+ Partials                         45       44       -1     
Files Coverage Δ
src/apps/KintoAppRegistry.sol 80.95% <ø> (ø)
src/paymasters/SponsorPaymaster.sol 75.00% <ø> (ø)
src/viewers/KYCViewer.sol 76.92% <ø> (ø)
src/wallet/KintoWallet.sol 86.36% <ø> (ø)
src/wallet/KintoWalletFactory.sol 62.96% <100.00%> (+1.85%) ⬆️

@rrecuero rrecuero merged commit b396cc0 into fix-paymaster-and-wallet Jan 23, 2024
3 of 5 checks passed
@rrecuero rrecuero deleted the 21th-migration branch January 23, 2024 21:36
@fedealconada fedealconada restored the 21th-migration branch January 23, 2024 21:41
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