Skip to content
This repository has been archived by the owner on Oct 6, 2023. It is now read-only.

Add manage:registrar:updateConfig script #240

Merged
merged 5 commits into from
Jul 26, 2023

Conversation

0xNeshi
Copy link
Contributor

@0xNeshi 0xNeshi commented Jul 26, 2023

Explanation of the solution

For some reason the Mumbai Registrar config's multisigFactory and multisigEmitter had addresses to old contracts and not to the newly deployed ones, causing Subgraph to not be able to catch events emitted from EndowmentMultiSigEmitter.
Created a script that updates Registrar's config to be able to update this manually.

Instructions on making this work

  • run yarn or yarn install to install npm dependencies
  • run yarn test to verify all tests still pass

@0xNeshi 0xNeshi added the bug Something isn't working label Jul 26, 2023
@0xNeshi 0xNeshi self-assigned this Jul 26, 2023
@0xNeshi
Copy link
Contributor Author

0xNeshi commented Jul 26, 2023

@stevieraykatz @SovereignAndrey I'm split between:

  1. making updateConfig update just the parameters passed to it while keeping all the other fields intact --> current solution
  2. making updateConfig update the parameters passed to it while getting all the other fields "up to date", i.e. read the most recent addresses from contract-address.json and use those values to update the config

The way I see it, both of these functions would be useful. So my proposal would be to keep things as-is (so point 1) above), but if/when we need the "get everything up to date" functionality, we do one of the following:

  • create a separate script for each contract (where applicable) --> we already have this for Registrar config, see manage/updateRegistrar
  • create one script that gets all contracts up-to-date with the current addresses in contract-address.json
  • add a flag --all (or --upgrade or whatever) to all applicable scripts that turns said scripts to version 2) above

@SovereignAndrey
Copy link
Contributor

SovereignAndrey commented Jul 26, 2023

@stevieraykatz @SovereignAndrey I'm split between:

1. making `updateConfig` update just the parameters passed to it while keeping all the other fields intact --> **current solution**

2. making `updateConfig` update the parameters passed to it while getting all the other fields "up to date", i.e. read the most recent addresses from _contract-address.json_ and use those values to update the config

My vote would be for option 1. Less "magic" and more straight-forward. What we pass in is what gets updated.

Update on magic: My fear around "magic" scripts (read: getting too fancy with script logic), is that it opens up greater possibilities for bugs when things change or we don't anticipate the scripts handling a certain types of inputs or grabs outdated input and updates that on the contracts. It's also more overhead to remember what a script is doing since we'll need to know when we run it. I would rather run a dead simple script 10 times to get the desired results than a more "magic" script 1 time and hope that everything works and there were no side effects, esp when it comes to the updating/modifying actions on the contracts.

@stevieraykatz stevieraykatz self-requested a review July 26, 2023 22:09
@stevieraykatz
Copy link
Contributor

Leaving open to finish discussion about magic.

@SovereignAndrey SovereignAndrey merged commit 61c18a7 into master Jul 26, 2023
1 check passed
@SovereignAndrey SovereignAndrey deleted the add-reg-update-config branch July 26, 2023 23:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants