makerdao / spells-mainnet

Staging repo for MakerDAO weekly executive spells
GNU Affero General Public License v3.0
108 stars 45 forks source link

2021-07-26 RWA003 - RWA006 Spell #173

Closed hieronx closed 3 years ago

hieronx commented 3 years ago

Description

Contribution Checklist

Checklist

Centrifuge Asset Checklist

A few notes about scope.

In scope:

Out of scope (unless you really want to):

checklist

RWA003

RWA004

RWA005

RWA006

spell-wide checklist

gbalabasquer commented 3 years ago

Please make sure src/DssSpell.t.sol has only the needed changes for this spell. I see several differences that should not exist (formatting, spaces, uint256 => uint, missing functions, missing or extra comments, etc). The diff should only show remotion of specific things of the prev spell and addendums of this specific new spell + the new collaterals in the structure. Whatever remains in this PR will be the base for the next ones so we should make sure the test file doesn't get altered in any case that doesn't correspond.

hieronx commented 3 years ago

Please make sure src/DssSpell.t.sol has only the needed changes for this spell. I see several differences that should not exist (formatting, spaces, uint256 => uint, missing functions, missing or extra comments, etc).

@gbalabasquer these differences you mention are the differences that exist between the spell tests in the maker kovan spells repo and the maker mainnet spells repo. @godsflaw has told us that we should keep the differences between the kovan and mainnet versions as small as possible, which is why we've put it together like this.

gbalabasquer commented 3 years ago

@gbalabasquer these differences you mention are the differences that exist between the spell tests in the maker kovan spells repo and the maker mainnet spells repo. @godsflaw has told us that we should keep the differences between the kovan and mainnet versions as small as possible, which is why we've put it together like this.

I guess he meant the test functions, but having the template changed shouldn't be the case. Each week we use the prev spell files as base for the new one. With this approach, the template is broken. Having minimum changes between spells is also helpful to detect exactly what is new in the system.

gbalabasquer commented 3 years ago

I just pushed a commit which reverts the changes that are not related to this spell.

kmbarry1 commented 3 years ago

Not sure where the others are with their reviews, but this pretty much LGTM except that we are waiting on the exec copy to be done so we can add the hash. There's also been some discussion that we might add the changes from last week here since the previous executive is having trouble passing.

talbaneth commented 3 years ago

dapp-test: rpc block: latest Running 16 tests for src/DssSpell.t.sol:DssSpellTest [PASS] testFail_notScheduled() (gas: 4759) [PASS] test_auth_in_sources() (gas: 281473904021917) [PASS] test_use_eta() (gas: 171113) [PASS] testOnTime() (gas: 6769842) [PASS] test_nextCastTime() (gas: 171955) [PASS] testFailTooEarly() (gas: 3501) [PASS] test_bytecode_matches() (gas: 2310700) [PASS] test_auth() (gas: 281473906858052) [PASS] testSpellIsCast_GENERAL() (gas: 13766183) [PASS] testManagerWards() (gas: 29565) [PASS] testFailWrongDay() (gas: 3655) [PASS] testCastCost() (gas: 6771456) [PASS] testNewChainlogValues() (gas: 6814928) [PASS] test_RWA_values() (gas: 8433612) [PASS] testNewIlkRegistryValues() (gas: 6849127) [PASS] testFailTooLate() (gas: 3853)

Running 2 tests for src/DssSpellManager.t.sol:DssSpellManagerTest [PASS] testWipeAndExit() (gas: 934898) [PASS] testJoinAndDraw() (gas: 592691)

godsflaw commented 3 years ago

Tests with changes for deployed spell:

Running 16 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testFail_notScheduled() (gas: 9759)
[PASS] test_auth_in_sources() (gas: 281473904021948)
[PASS] test_use_eta() (gas: 322773)
[PASS] testOnTime() (gas: 6921502)
[PASS] test_nextCastTime() (gas: 323615)
[PASS] testFailTooEarly() (gas: 8501)
[PASS] test_bytecode_matches() (gas: 2317700)
[PASS] test_auth() (gas: 281473906858084)
[PASS] testSpellIsCast_GENERAL() (gas: 16222006)
[PASS] testManagerWards() (gas: 29565)
[PASS] testFailWrongDay() (gas: 8655)
[PASS] testCastCost() (gas: 6923116)
[PASS] testNewChainlogValues() (gas: 6966588)
[PASS] test_RWA_values() (gas: 8585272)
[PASS] testNewIlkRegistryValues() (gas: 7000787)
[PASS] testFailTooLate() (gas: 8853)

Running 2 tests for src/DssSpellManager.t.sol:DssSpellManagerTest
[PASS] testWipeAndExit() (gas: 934898)
[PASS] testJoinAndDraw() (gas: 592691)
hieronx commented 3 years ago

Tests pass on deployed spell--will approve on merging of deployed spell commit and archival.

Just archived it @kmbarry1