GEN-2786: sunset script #365

fedealconada · 2023-10-15T22:13:08Z

Motivation

Sunset V1

Solution

The script gets all the contracts we've ever deployed from Sense's admin addresses and:

removes ourselves from being a rewards recipient (for contracts with ExtractableReward.sol
removes the trust to these addresses (for contracts with Trust.sol

Contracts with the deployer address trusted don't need to be executed via multisig.
For all the rest of the transactions, the script will output a JSON file with all the txs and its data to execute them using the Gnosis Transaction Builder (batched).

codecov · 2023-10-16T12:22:22Z

Codecov Report

Merging #365 (68064a4) into dev (2993464) will not change coverage.
The diff coverage is n/a.

❗ Current head 68064a4 differs from pull request most recent head 4964b70. Consider uploading reports for the commit 4964b70 to get more accurate results

@@           Coverage Diff           @@
##              dev     #365   +/-   ##
=======================================
  Coverage   67.28%   67.28%           
=======================================
  Files          43       43           
  Lines        1137     1137           
  Branches      214      214           
=======================================
  Hits          765      765           
  Misses        285      285           
  Partials       87       87

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jparklev

A few notes & questions, mostly on the removeTrust paths, but overall this is a nice approach!!

Pending resolution on the RLV adapter depreciation discussion and the review comments, LGTM

jparklev · 2023-10-16T16:55:29Z

pkg/deployments/tasks/20231031-sunset/index.js

+const calculateChecksum = batchFile => {
+  const serialized = serializeJSONObject({
+    ...batchFile,
+    meta: { ...batchFile.meta, name: null },


why does the name have to be set as null?

it was just a copy paste to be honest

jparklev · 2023-10-16T16:57:16Z

pkg/deployments/tasks/20231031-sunset/index.js

+                (tx.type === "create" || tx.type === "create2") &&
+                tx.contractAddress !== address,
+            )
+            .map(async tx => {


tiny nit: doesn't need to be async if we're not using getContractName

jparklev · 2023-10-16T16:59:28Z

pkg/deployments/tasks/20231031-sunset/index.js

+    if (Object.values(SENSE_ADMIN_ADDRESSES).includes((await contract.rewardsRecipient()).toLowerCase())) {
+      // some contracts were never changed to be trusted by the multisig and remained trusted by the deployer
+      if (!(await contract.isTrusted(SENSE_ADMIN_ADDRESSES.multisig))) {
+        contract = contract.connect(deployerSigner);


It seems like we update the connected contract address, but then it's never used (due to the if else)

jparklev · 2023-10-16T17:04:50Z

pkg/deployments/tasks/20231031-sunset/index.js

+
+      await contract.setIsTrusted(address, false, { gasPrice: boostedGasPrice() }).then(t => t.wait());
+
+      if (address.toLowerCase() === SENSE_ADMIN_ADDRESSES.multisig.toLowerCase()) {


So this will only trigger if address is the SENSE_ADMIN_ADDRESSES, and the address list does include the deployer, right? Since line 188 will continue if the address list doesn't include the deployer.

I might be reading it wrong, but i'd expect this to trigger only if the address list doesn't include the deployer, since otherwise we don't need to use multisig, right?

i think this was a bit "complex" the way it as. I've simplified it by addin logic to the getTrustedAddresses() so it now returns the trusted addresses array but placing the multisig and deployer (if they are trusted) at the end of the array. If both are trusted, deployer address must be the last one. So, then, when iterating over it, we leave the corresponding deployer to remove trust to itself in last place.

jparklev · 2023-10-16T17:05:11Z

pkg/deployments/tasks/20231031-sunset/index.js

+
+    for (const address of trusted.sense) {
+      if (includesDeployer && address === SENSE_ADMIN_ADDRESSES.deployer.toLowerCase()) continue;
+      if (!includesDeployer && address === SENSE_ADMIN_ADDRESSES.multisig.toLowerCase()) continue;


does address need to be toLowerCase like line 192

i've polished all the toLowerCase()

jparklev · 2023-10-16T17:08:52Z

pkg/deployments/tasks/20231031-sunset/index.js

+        .setIsTrusted(SENSE_ADMIN_ADDRESSES.deployer, false, { gasPrice: boostedGasPrice() })
+        .then(t => t.wait());
+    } else {
+      await contract


Just to make sure I understand – this will fail on mainnet, right? Since we can't use the multisig signer. But we run it on hardhat and get the multisig txs list there? And do we also run this script on mainnet to get the deployer txs out

haha, good call, i've just fixed it so we can run it on mainnet (for the deployer ones and skipping the multisig ones)

stevenvaleri

Two high-level items that may make sense:

Add test scripts to confirm the action results.
Separate the remove rewards recipient step and the remove trust step, that way we can remove the recipients, confirm they are removed, and then remove the trust separately. I think it's quite unlikely, but maybe if there were an error in removing the reward recipient, but we remove the trust anyway, the reward recipient it could be stuck.

stevenvaleri · 2023-10-16T18:42:19Z

pkg/deployments/tasks/20231031-sunset/index.js

+        "setPermissionless",
+        divider.interface.encodeFunctionData("setPermissionless", [true]),
+      ),
+    );


Does this call it both directly here and add to the multi-sig tx array? Seems like it's one or the other in the other scenarios.

not sure i follow you, the divider is trusted by multisig, so we need to execute this function from it by adding it to the batch

My confusion is on line 53 - where the divider is called directly, before adding it to the multi-sig txs. Is that only intended for the instance when we are running this on a fork?

ohh, yes yes, it was only to test on fork, i've already included the changes to skip if run on mainnet (otherwise it would fail)

jparklev · 2023-10-17T00:52:14Z

Also @fedealconada since you're off wed – once you've addressed these comments and are happy with it, feel free to start running the script and generating the multisig json :)

(since looks like we have a resolution on what to do w the RLVs in #ops-internal ~)

fedealconada · 2023-10-17T10:12:38Z

Two high-level items that may make sense:

Add test scripts to confirm the action results.

Separate the remove rewards recipient step and the remove trust step, that way we can remove the recipients, confirm they are removed, and then remove the trust separately. I think it's quite unlikely, but maybe if there were an error in removing the reward recipient, but we remove the trust anyway, the reward recipient it could be stuck.

good point, it's always good to be safe. just a q: you are saying we run the batch of txs on Gnosis, then sort of run a sanitiy check to ensure everything is right, and then prepare another batch of txs for the trust removal?

i've now added (2) and also sanity checks but still generating one batch tx with everything in it.

fedealconada · 2023-10-17T14:48:10Z

Also @fedealconada since you're off wed – once you've addressed these comments and are happy with it, feel free to start running the script and generating the multisig json :)

(since looks like we have a resolution on what to do w the RLVs in #ops-internal ~)

you mean i should already run the script to do the deployer actions now? just to clarify because i think @KentonPrescott said to wait until Friday (btw, I'll be on PTO but can execute this any day)

jparklev · 2023-10-17T14:50:02Z

you mean i should already run the script to do the deployer actions now?

Yes! Oh, yea I think kenton just meant that we would announce on friday, and he was wanting to get your help before you went on PTO. But! If you'll be able to execute any day it's all good!

fedealconada · 2023-10-17T14:52:44Z

you mean i should already run the script to do the deployer actions now?

Yes! Oh, yea I think kenton just meant that we would announce on friday, and he was wanting to get your help before you went on PTO. But! If you'll be able to execute any day it's all good!

nah, i can run it today then, will do and let you know!

stevenvaleri · 2023-10-17T16:01:53Z

just a q: you are saying we run the batch of txs on Gnosis, then sort of run a sanitiy check to ensure everything is right, and then prepare another batch of txs for the trust removal?

yes - run a batch to remove reward recipients, sanity check, run a batch to remove trust

fedealconada requested review from KentonPrescott, jparklev and stevenvaleri October 15, 2023 22:13

fedealconada added 4 commits October 16, 2023 12:55

feat: add Sense v1 sunset script

5eb82d0

feat: add multisigTxs.json

39c3403

feat: add create2 txs and checksum for Gnosis compatbility

6a1b18f

chore: add yarn.lock

0ef4011

fedealconada force-pushed the GEN-2786-sunset branch from 8e05207 to 0ef4011 Compare October 16, 2023 11:55

jparklev reviewed Oct 16, 2023

View reviewed changes

stevenvaleri approved these changes Oct 16, 2023

View reviewed changes

chore: add changes from PR review

68064a4

refactor: sunset

4964b70

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEN-2786: sunset script #365

GEN-2786: sunset script #365

fedealconada commented Oct 15, 2023 •

edited

Loading

codecov bot commented Oct 16, 2023 •

edited

Loading

jparklev left a comment •

edited

Loading

jparklev Oct 16, 2023

fedealconada Oct 17, 2023

jparklev Oct 16, 2023

jparklev Oct 16, 2023

jparklev Oct 16, 2023

fedealconada Oct 17, 2023

jparklev Oct 16, 2023 •

edited

Loading

fedealconada Oct 17, 2023

jparklev Oct 16, 2023 •

edited

Loading

fedealconada Oct 17, 2023

stevenvaleri left a comment

stevenvaleri Oct 16, 2023

fedealconada Oct 17, 2023

stevenvaleri Oct 17, 2023

fedealconada Oct 17, 2023

jparklev commented Oct 17, 2023

fedealconada commented Oct 17, 2023

fedealconada commented Oct 17, 2023

jparklev commented Oct 17, 2023 •

edited

Loading

fedealconada commented Oct 17, 2023

stevenvaleri commented Oct 17, 2023


		await contract.setIsTrusted(address, false, { gasPrice: boostedGasPrice() }).then(t => t.wait());

		if (address.toLowerCase() === SENSE_ADMIN_ADDRESSES.multisig.toLowerCase()) {

GEN-2786: sunset script #365

Are you sure you want to change the base?

GEN-2786: sunset script #365

Conversation

fedealconada commented Oct 15, 2023 • edited Loading

Motivation

Solution

codecov bot commented Oct 16, 2023 • edited Loading

Codecov Report

jparklev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jparklev Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jparklev Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenvaleri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jparklev commented Oct 17, 2023

fedealconada commented Oct 17, 2023

fedealconada commented Oct 17, 2023

jparklev commented Oct 17, 2023 • edited Loading

fedealconada commented Oct 17, 2023

stevenvaleri commented Oct 17, 2023

fedealconada commented Oct 15, 2023 •

edited

Loading

codecov bot commented Oct 16, 2023 •

edited

Loading

jparklev left a comment •

edited

Loading

jparklev Oct 16, 2023 •

edited

Loading

jparklev Oct 16, 2023 •

edited

Loading

jparklev commented Oct 17, 2023 •

edited

Loading