search

makerdao / spells-mainnet

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

Move collateral onboarding #212

Closed iamchrissmith closed 2 years ago

iamchrissmith commented 2 years ago

move collateral onboarding to a separate file structure to avoid conflicts when multiple teams are working on the weekly executive

iamchrissmith commented 2 years ago

Tests with template option:

Running 13 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testFail_notScheduled() (gas: 7161)
[PASS] test_auth_in_sources() (gas: 281473904838271)
[PASS] test_use_eta() (gas: 326798)
[PASS] testOnTime() (gas: 2688502)
[PASS] testVestTransfer() (gas: 4063768)
[PASS] test_nextCastTime() (gas: 327693)
[PASS] testFailTooEarly() (gas: 3528)
[PASS] test_bytecode_matches() (gas: 2783322)
[PASS] test_auth() (gas: 281473913169994)
[PASS] testSpellIsCast_GENERAL() (gas: 11536985)
[PASS] testFailWrongDay() (gas: 3572)
[PASS] testCastCost() (gas: 2690113)
[PASS] testFailTooLate() (gas: 3549)

Tests with empty inheritance option: (Import the CollateralActions file and inherit it in the SpellActions contract but do not call the onboard action function)

[PASS] testFail_notScheduled() (gas: 7161)
[PASS] test_auth_in_sources() (gas: 281473904838271)
[PASS] test_use_eta() (gas: 326798)
[PASS] testOnTime() (gas: 2688502)
[PASS] testVestTransfer() (gas: 4063768)
[PASS] test_nextCastTime() (gas: 327693)
[PASS] testFailTooEarly() (gas: 3528)
[PASS] test_bytecode_matches() (gas: 2783322)
[PASS] test_auth() (gas: 281473913169994)
[PASS] testSpellIsCast_GENERAL() (gas: 11536985)
[PASS] testFailWrongDay() (gas: 3572)
[PASS] testCastCost() (gas: 2690113)
[PASS] testFailTooLate() (gas: 3549)
iamchrissmith commented 2 years ago

@brianmcmichael and @naszam based on my testing of the difference from having Collateral onboarding in a template file VS keeping it in the main src directory and inheriting the empty contract to DssSpellAction I am not seeing any difference in gas usage in the tests.

Do you have an idea for how I could benchmark that differently? or if it does not matter from a gas perspective, do you two have a preference how it is organized?

brianmcmichael commented 2 years ago

@brianmcmichael and @naszam based on my testing of the difference from having Collateral onboarding in a template file VS keeping it in the main src directory and inheriting the empty contract to DssSpellAction I am not seeing any difference in gas usage in the tests.

Do you have an idea for how I could benchmark that differently? or if it does not matter from a gas perspective, do you two have a preference how it is organized?

I suspected this may be the case. Since there's no code paths the compiler just won't add anything related to the contract to the bytecode. Leaving it in and empty is my preference because of the way we typically review spells by examining diffs.

iamchrissmith commented 2 years ago

@brianmcmichael and @naszam based on my testing of the difference from having Collateral onboarding in a template file VS keeping it in the main src directory and inheriting the empty contract to DssSpellAction I am not seeing any difference in gas usage in the tests. Do you have an idea for how I could benchmark that differently? or if it does not matter from a gas perspective, do you two have a preference how it is organized?

I suspected this may be the case. Since there's no code paths the compiler just won't add anything related to the contract to the bytecode. Leaving it in and empty is my preference because of the way we typically review spells by examining diffs.

I made that change and pushed it, let me know how it feels