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

Dewiz x Sidestream x Arby Spell Improvements #12

Merged
merged 143 commits into from
Oct 25, 2023

Conversation

The-Arbiter
Copy link
Collaborator

Hi,

Changes as required from my 2023-05-10 Goerli review.

More to come as necessary.

Copy link
Contributor

@SidestreamColdMelon SidestreamColdMelon left a comment

Choose a reason for hiding this comment

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

Changes are looking good from our side and doesn't contain anything we haven't discussed

spell/spell-reviewer-mainnet-checklist.md Outdated Show resolved Hide resolved
spell/rwa-onboarding-checklist.md Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented May 9, 2023

For complete clarity this is the GovAlpha perspective:

  • ES is a core component of the game theory in the MCD design used by the Maker Protocol (see: whitepaper).
  • Failure to appropriately authorise the ESM on new contracts can lead to a failure mode in the event ES is called, with potential significant downsides for DAI holders as a result.
  • We have recently paid out big bug bounties as a result of the identification of ES issues.
  • It is, therefore, in my opinion, fine from a governance perspective to assume that the ESM should be authed for new contracts unless explicitly stated otherwise.

That said, things may change if, at some point, ES is removed from being a core component of the Protocol, and this should be reviewed if and/or when that happens.

@SidestreamColdMelon
Copy link
Contributor

A suggestion from LongForWisdom | GovAlpha from the #new-spells discord channel: move Fill Spell Crafter Related Boxes in the Main Exec Doc Sheet point to much earlier in the process.

@LongForWisdom
Copy link

So, there are three sets of things to confirm on the sheet.

  1. (Contents sheet) 'Spellcrafter Confirm Content is feasible' - Ideally, this should be filled in shortly after GovAlpha creates the content sheet for the spell. Likely on Wednesday. Confirmation here indicates that the crafter understands and accepts the desired contents for the spell.
  2. (Contents sheet) 'Spellcrafter confirm content in spell' - Ideally, this should be filled in after the mainnet spell is deployed? I guess? Open to feedback on when this should happen.
  3. (Main sheet) The rows in green. We should discuss what should be included in the main sheet related to technical spell work. I suspect it's largely useless to you guys. For us it could be a good way to see what stage the spell is at without bothering you.

The spell sheet itself has not been brought up to date recently. There are a number of rows that are no longer relevant, and the 'When?' column was never updated for the new schedule. We should discuss, and then GovAlpha should do an update pass on it.

@The-Arbiter
Copy link
Collaborator Author

I agree. Let's sync up on this relatively soon so that we can balance overhead / effort expended with transparency. IMO we used to be a little slack on this due to the nature of content coming in late-ish or perhaps just due to the cadence.

Having one "official spell comms channel" might be useful when it comes to these kinds of things too (re: you guys knowing where spells are at). Spell crafters determining feasibility of the content is a default yes (IMO) unless the content comes in late, since practically all types of content can be done, it's simply a matter of what gets done when. Pushback on here would rarely happen via sheet, I would assume. So you know, typically (obviously this will likely change with the introduction of subDAOs) all spell content is completed concurrently, with reviews (ideally) covering all included content at once in order to minimize the risk of something sneaking in through a later commit.

Happy to explore all the options available here with you and other spell participants at some point in the future; if you have any feedback or recommendations they are always appreciated!

@The-Arbiter The-Arbiter changed the title 2023-05-10 spell improvements Dewiz x Sidestream x Arby Spell Improvements May 24, 2023
Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

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

Let's get this done!

@SidestreamSweatyPumpkin SidestreamSweatyPumpkin merged commit 2c635a7 into master Oct 25, 2023
@amusingaxl amusingaxl deleted the 2023-05-10-improvements branch October 25, 2023 13:06
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.

6 participants