Emergency 2023-06-16: Coinbase Custody DAO Resolution #347

amusingaxl · 2023-06-16T13:12:48Z

Description

Contribution Checklist

PR title starts with (PE-<TICKET_NUMBER>)
Code approved
Tests approved
CI Tests pass

Checklist

src/DssSpell.t.sol

The-Arbiter · 2023-06-16T15:08:40Z

src/DssSpell.t.sol

@@ -361,41 +361,41 @@ contract DssSpellTest is DssSpellTestBase {
        assertTrue(lerp.done());
    }

-    function testNewChainlogValues() public { // don't disable
+    function testNewChainlogValues() private { // amke private to disable


Leave this test private but change this comment back.

The comment was changed here:
https://github.com/makerdao/spells-mainnet/pull/339/files#diff-cd597c47b9a6e2bae57aab767ee9e3d5bb3d8edeb3a2248b4519b9af791cb468R302

It's not a directive to prevent people from disabling it, it was just stating that the test needed to be enabled for that specific spell.

The-Arbiter · 2023-06-16T15:10:09Z

src/DssSpell.sol

+        "2023-06-16 MakerDAO Executive Spell | Hash: TODO";
+
+    // Comma-separated list of DAO resolutions.
+    string public constant RESOLUTIONS = "QmYPbYoPjrSzBu5gt9tip78siG1gFvY8K4HTkHEFJgMmM8";


I think there is probably a better name for this, but either way we normally use lower case for string public constant per the archives.

The-Arbiter · 2023-06-16T15:16:25Z

There are almost 50 lines of unused interfaces from the old spell at the top of the spell file.

Please finish crafting the spell before marking it as ready for review.

The-Arbiter · 2023-06-16T15:21:38Z

Unused imports not removed
Unused interfaces not removed
Nit: An incorrect raw.githubusercontent.com URL is present in the exec hash comment instead of any TODO (though admittedly this may not be ready yet, we need to mark it TODO so we don't forget to change it)
Test comment for the test that has always been commented as "don't disable" has been changed to "amke private to disable" [sic] <-- Ignoring the typo, this is not the time or place to be changing how we treat tests that have been marked clearly as "don't disable".

Focus on cleaning up the spell and then the new content for the emergency spell once the cleanup is finished.

As it stands, this spell is half-baked and not ready for review. The reviewers job is not to finish half of the crafting process; this shouldn't have been marked as ready for review until it had been looked over by the crafter at least once, which would've made evident things like half of the spell file being unused interfaces from the last spell.

The-Arbiter

Will fully review according to the checklist after these issues have been fixed:

Seven unused interfaces in tests file
When removing a test, sometimes interfaces added for this test will no longer be used. These should be removed as part of cleanup.
(Seven includes JugLike, which seems to have been missed in the past, this fix is optional)
testNewChainlogValues changes to be reverted (see relevant comment)
testNewIlkRegistryValues can be cleaned up in on of two ways, which I have suggested
testCollateralIntegration can also be cleaned up or not, I am fine to leave it as it is not cleanup from the spell directly before the current one, but I have left notes for treatment of this test per the checklist.

The-Arbiter · 2023-06-16T17:15:20Z

src/DssSpell.sol

+        "2023-06-16 MakerDAO Executive Spell | Hash: 0xecdc5a5a1be69f23d299235ad966c82c7e03365e45aaed69ed1eb2f03daff4a6";
+
+    // Comma-separated list of DAO resolutions.
+    string public constant resolutions = "QmYPbYoPjrSzBu5gt9tip78siG1gFvY8K4HTkHEFJgMmM8";


// ----- 2023-06-16 DAO resolutions ----- // Forum: https://forum.makerdao.com/t/out-of-schedule-executive-to-fortify-elasticity-of-on-chain-1-1-liquidity-of-usdc/21168 string public constant dao_resolution = "QmYPbYoPjrSzBu5gt9tip78siG1gFvY8K4HTkHEFJgMmM8";

Semantics here but this is how I would do it personally given that this is the point of the spell (as a direct action).

Made some changes.

At some point, we might need more than 1 resolution in a single spell.
This hasn't been done before, so I'm setting the precedent right now.
Since we can't have string[] as constant, we can simply stick to a comma-separated list of IFPS hashes, as it is purely informational.

I somewhat like this structure, let's do this if and when we have multiple resolutions in a spell rather than confusing people reading this original code. My only rebuttal to the structure here would be that we may want clarity as to which resolution is which, but again, that's an issue for another time.

I won't object to a more confusing naming structure if that's what you elect to use as it is functionally identical though I do think that having a plural for the first ever instance of things and deciding on a precedent in an emergency spell is probably not the best idea. Non-functional anyway so this isn't blocking.

src/DssSpell.sol

The-Arbiter · 2023-06-16T17:27:03Z

src/DssSpell.t.sol

@@ -361,7 +361,7 @@ contract DssSpellTest is DssSpellTestBase {
        assertTrue(lerp.done());
    }

-    function testNewChainlogValues() public { // don't disable
+    function testNewChainlogValues() private { // make private to disable


Please follow precedent with regards to this function and revert all changes which alter the behavior of this test, as this is unrelated to the content of this (emergency) spell.

Precedent to follow:
2023-01-11
2023-05-10
2023-05-17

Please reference the procedure for dealing with this in the checklist if there are other concerns. It is my opinion that the objectively menacing sounding comment next to this test suggests that this treatment is necessary for this test, which is clearly an opinion shared by multiple crafters in the past given the precedent.

Checklist says nothing specific about comments.

This change was first introduced here:
https://github.com/makerdao/spells-mainnet/pull/339/files#diff-cd597c47b9a6e2bae57aab767ee9e3d5bb3d8edeb3a2248b4519b9af791cb468R302

This is not a directive to further crafters to not disable this test, it's simply describing what it's doing.

Looking past that, the full history of the archive has the updated comment, which is in line with every other test that can be disabled.

See:

2023-01-11

2022-11-12

2022-11-02

Before that, the practice was to toggle between // make public to use and // make private to disable, but we abandoned that practice.

See:

2022-09-14

This comment has been misinterpreted and carried over for the past 5 spells.
The test is redundant in all those spells you mentioned. The chainlog version is already checked here.

In reality, the 5 last spells failed to do the proper clean up as per the checklist.

An emergency spell is not the context to be introducing test changes where they are not relevant to input or derived actions for that spell.

Test maintenance has historically been done during maintenance PRs or (in cases where the test is blocking certain spell content) test maintenance for a specific test can be done where the spell team unanimously agrees that the test update fits the following criteria.

In this case, this test is not pertinent to the content of the 2023-06-16 spell (which does not contain chainlog changes, new chainlog values or a version bump) and therefore it would generally be considered wise to follow the principle of "if it ain't broke, don't fix it".

The crafter checklist for mainnet spells (which you have not posted) contains pretty clear instructions:

Simply following this instruction would make the test pass and remove any requirement for making it private. Admittedly the checklist appears to treat all of these options as weighted equally, so this definitely needs clarification in the future. Obviously the 'disable by setting to private' option here clashes with the comment 'don't disable' and so the method to make the test pass (with sufficient coverage) that doesn't involve making it private is clearly what the checklist is instructing you to do.

My personal procedure for spell cleanup is to figure out what was added by the last spell and remove these elements, following archive patterns in order to leave barebones code / comments where necessary. Removing the chainlog checks for these chainlog keys and leaving the version check for 1.14.13 will make the test pass.

I prioritize security and if a test says don't disable I'm not going to approve a hastily made change to it within a 24 hour SLA emergency spell, regardless of whether this comment makes logical sense or not.

As the crafter for this spell, your role is to ship the instruction set provided by Governance Facilitators as spell code. Perhaps you are correct, but harking back to the first line, this is not the context for this change as it is not at all relevant to the current spell's content.

All that debating about this is doing is delaying the shipping of an emergency spell over an extraneous (functional) change which I am not comfortable with shipping given our current context. Note that my objection to this change does not block spell content nor does it affect any test coverage related to the spell content and is rooted in a strong prioritization of security.

It's not a functional change, it's a comment. More important than that, it's a wrong comment.

This is literally an empty spell, I've shown you why the comment is wrong, I don't get why you need to make a fuss about it.

Don't be the boy who cried wolf, otherwise people will not take you seriously when something is actually wrong.

I have no objection to fixing the code in the future (in fact, your points as outlined above make sense and I would support making an update to our test setup for chainlog checks). In fact, quite a lot of our tests need updating, maintenance and hardening.

However, describing disabling a test marked don't disable as "not a functional change" is drastically incompetent at best and actively malicious at worst. You are disabling a function. At best, this is functionally equivalent due to coverage elsewhere, but at worst this leads to a lack of coverage from this spell onwards until someone catches it in the future.

Either way, this is not something I am comfortable shipping with such short notice given that it has nothing to do with the current spell's content, so I cannot resolve this issue while a functional change that I disagree with is present, even if it's just checking the chainlog version. I am not approving this in the same way that I would not approve disabling testPSMs (which is another test with a note on it to be left as enabled / public) in a emergency spell. It's not a matter of whether you're right or wrong and more a matter of this being the wrong place and time for this kind of change.

However, describing disabling a test marked don't disable as "not a functional change" is drastically incompetent at best and actively malicious at worst.

You are joking, right? This must be a joke. Where are the cameras?

You are disabling a function. At best, this is functionally equivalent due to coverage elsewhere, but at worst this leads to a lack of coverage from this spell onwards until someone catches it in the future.

And removing the content of the test is what besides disabling it?

The only thing present in the test now is checking the chainlog version, which is already done in other test.

If you spent half of the energy you spent writing these comments simply going through what I highlighted, we wouldn't be having this discussion.

And removing the content of the test is what besides disabling it?

Precedent leaves version check, see archives, it isn't empty.

which is already done in other test.

Yes, this is probably true. That's why this gets fixed when spell tests are being maintained or upgraded and not in an emergency spell, since this has nothing to do with declaring a DAO Resolution.

Precedent leaves version check, see archives, it isn't empty.

This is false. There are only 2 instances in which this was done like that:

2023-05-10
2023-05-17

Every single other spell in the archive which contains this test is either active (meaning there are new chainlog entries to test) or marked private.

src/DssSpell.t.sol

src/test/config.sol

The-Arbiter · 2023-06-16T18:58:19Z

src/DssSpell.sol

-    string public constant resolutions = "QmYPbYoPjrSzBu5gt9tip78siG1gFvY8K4HTkHEFJgMmM8";
+    // ----- 2023-06-16 DAO Resolutions -----
+    // Forum: https://forum.makerdao.com/t/out-of-schedule-executive-to-fortify-elasticity-of-on-chain-1-1-liquidity-of-usdc/21168
+    // Comma-separated list of DAO resolutions IPFS hashes.


Apologies if I'm missing something here but doesn't this point to one IPFS document (a file named WithdrawalCoinbaseCustody.docx)?

If this is the case then this line should be removed but perhaps the comment could be adapted, as it serves to add context to people reading the code.

The 2023-03-11 spell used this comment:

// Emergency Proposal: Risk and Governance Parameter Changes (11 March 2023)

So (up to you) perhaps we could use // Emergency Proposal: James Asset Trust 3 Withdrawal Process Of USDC From Coinbase Custody (16 June 2023) in order to match this kind of comment and add some context to what this document is?

Added the specific comment about Emergency Proposal, but kept everything else for context.

Please take note that emergency spells are not the place to innovate or set precedents. Emergency spells are (as the name would suggest) about shipping content in an emergency. Setting precedent here (given short timeframes and special crafter / reviewer setups often used for these kinds of things) doesn't sit right with me personally, but it's non-functional and at worst it's just going to confuse whoever reads the comment (which is what it clearly did to me) so I am resolving this issue even though I don't agree with this change.

src/DssSpell.sol

src/DssSpell.t.sol

src/DssSpell.sol

The-Arbiter

Mainnet Executive Spell Review Checklist

Mainnet 2023-06-16 (EMERGENCY SPELL)

Spell Actions:

Publish James Asset Trust (3) DAO Resolution onchain

Implied action: Cleanup previous spell

NOTE: It may be necessary to backpropagate spell cleanup to Goerli in order to prevent future work caused by spells-goerli and spells-mainnet being unsynchronized. This can be determined after the Emergency Spell SLA period.

Coding Stage

Arby's Additional IPFS Doc Checks

I have done these voluntarily and they do not represent a guarantee of information being correct.

Exec doc wording for JAT3 deal matches IPFS uploaded document OK
Document value for RWA014_A_INPUT_CONDUIT_URN matches ChainLog value OK (0x6B86bA08Bd7796464cEa758061Ac173D0268cf49)
Document value for RWA014_A_INPUT_CONDUIT_JAR matches ChainLog value OK (0x391470cD3D8307AdC051d878A95Fa9459F800Dbc)

./scripts/test-dssspell-forge.sh match="" block=""
Using DssExecLib at: 0x8De6DDbCd5053d32292AAA0D2105A32d108484a6
[⠃] Compiling...
[⠔] Compiling 5 files with 0.8.16
[⠔] Solc 0.8.16 finished in 2.62s
Compiler run successful!

Running 2 tests for src/test/starknet.t.sol:StarknetTests
[PASS] testStarknet() (gas: 532141)
[PASS] testStarknetSpell() (gas: 2346)
Test result: ok. 2 passed; 0 failed; finished in 153.25s

Running 18 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 9223371487104983364)
[PASS] testAuthInSources() (gas: 9223371487099443644)
[PASS] testBytecodeMatches() (gas: 1368596)
[PASS] testCastCost() (gas: 385625)
[PASS] testChainlogValues() (gas: 9035817)
[PASS] testChainlogVersionBump() (gas: 3927458)
[PASS] testContractSize() (gas: 8984)
[PASS] testDeployCost() (gas: 1357000)
[PASS] testFailNotScheduled() (gas: 14397)
[PASS] testFailTooEarly() (gas: 13607)
[PASS] testFailTooLate() (gas: 13606)
[PASS] testFailWrongDay() (gas: 13585)
[PASS] testGeneral() (gas: 34989734)
[PASS] testNewChainlogValues() (gas: 385238)
[PASS] testNextCastTime() (gas: 353636)
[PASS] testOnTime() (gas: 373217)
[PASS] testPSMs() (gas: 1863214)
[PASS] testUseEta() (gas: 352368)
Test result: ok. 18 passed; 0 failed; finished in 2484.84s

Good to deploy

Co-authored-by: Arby <103920908+The-Arbiter@users.noreply.github.com>

This reverts commit bb6a52b. Spell was not broken, but my local environment was running tests with the wrong configurations. Tests are passing on CI.

The-Arbiter · 2023-06-16T22:35:32Z

Noting here that the deployed version of the spell does not match the latest commit.

Typically this would be a blocking change and require a redeploy however this appears to only be a missing line of provenance comment (the MIP). I will check more in-depth to make sure that the diff is only limited to this line, in which case this will be non-blocking (as the comment is non-functional and serves as supporting rather than primary provenance).

The-Arbiter · 2023-06-16T22:45:09Z

Spell is not archived yet.

Please archive while my tests run.

The-Arbiter

Deployed Stage

Emergency spell total response time 18-24 hours from initial request.

./scripts/test-dssspell-forge.sh match="" block=""
Using DssExecLib at: 0x8De6DDbCd5053d32292AAA0D2105A32d108484a6
[⠊] Compiling...
No files changed, compilation skipped

Running 2 tests for src/test/starknet.t.sol:StarknetTests
[PASS] testStarknet() (gas: 532141)
[PASS] testStarknetSpell() (gas: 2346)
Test result: ok. 2 passed; 0 failed; finished in 218.86s

Running 18 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 9223371487104983364)
[PASS] testAuthInSources() (gas: 9223371487099443644)
[PASS] testBytecodeMatches() (gas: 1368596)
[PASS] testCastCost() (gas: 385625)
[PASS] testChainlogValues() (gas: 9035817)
[PASS] testChainlogVersionBump() (gas: 3927458)
[PASS] testContractSize() (gas: 8984)
[PASS] testDeployCost() (gas: 1357000)
[PASS] testFailNotScheduled() (gas: 14397)
[PASS] testFailTooEarly() (gas: 13607)
[PASS] testFailTooLate() (gas: 13606)
[PASS] testFailWrongDay() (gas: 13585)
[PASS] testGeneral() (gas: 34991831)
[PASS] testNewChainlogValues() (gas: 385238)
[PASS] testNextCastTime() (gas: 353636)
[PASS] testOnTime() (gas: 373217)
[PASS] testPSMs() (gas: 1863214)
[PASS] testUseEta() (gas: 352368)
Test result: ok. 18 passed; 0 failed; finished in 1710.31s

Second approval will be required for merge to main branch of spells-mainnet.
Backport can be done according to PE1204 precedent as necessary to sync Goerli and Mainnet states and repo code.

Good for handover.

amusingaxl added 3 commits June 16, 2023 10:08

chore: cleanup

30f5f91

feat: add DAO resolutions list

c80aaee

fix: disable office hours

fe2c68e

amusingaxl requested a review from The-Arbiter June 16, 2023 13:12

fix: disable office hours in config

d29c41d