[EPIC] Leftovers from phase b

[theethernaut]

Tasks

• [x] Finish test coverage
  • MERGED https://github.com/Synthetixio/synthetix/pull/804 - Takes SecondaryDeposit coverage to 94.59 (+4.85%)
• [x] (Anton) Split snapshots from Issuer
  • MERGED https://github.com/Synthetixio/SIPs/pull/269
  • https://github.com/Synthetixio/synthetix/pull/811 , waiting to be reviewed!
• [x] Moved to EPIC #135 ~Send 2.2m SNX to deposit (2 options):~
  • Send directly to SecondaryDeposit and then migrate.
  • Send directly to SynthtixOptimisticBridge (the new contract that deposit is going to be migrated to.
• [x] Moved to EPIC #133 ~Tests in OVM mode (the ones that are super slow)~
  • ~down to one hour (needs the new V2 update)~
• [x] Moved to EPIC #133 ~Multi/docker tests~
  • ~Fixed the same-chain ones instead.~
  • ~Kevin's: https://github.com/Synthetixio/synthetix/pull/787~
• [x] ~Add MintableSynthetix unit tests.~
  • (MERGED) https://github.com/Synthetixio/synthetix/pull/803

[theethernaut]

@i-stam a couple of things since you own this epic:

1) Let me suggest that Tasks in epics are easily measurable goals, so that we can make them finite. E.g. "Finish test coverage" could be "Take all L2 bridge related contracts to > 90% test coverage". I'm suggesting 90% because I think it makes more sense to hit > 95% in phase c, when things are more mature.

2) I just took a look at hitting the two internal functions in SecondaryDeposit.sol that aren't yet covered, namely

• function synthetix() internal view returns (ISynthetix)
• function rewardEscrow() internal view returns (IRewardEscrow) I think the latter we can just comment it out, because no external code uses it yet. The first one is super tricky because it raises the question of doing what test:multi-same-chain does, but here. To hit it, I would have to be able to call mintSecondaryDeposit without reverting in the initial require statements, which I could easily fake, but, we already do that in the unit tests right? Faking it here wouldn't make this a very good integration test. So, one thing we could do is bring in the test:multi-same-chain here, and get rid of it, since that test does hit the function but is not measured in coverage. A lot of work for just hitting one additional line. We should discuss this and choose what to do.

[i-stam]

1. Since most of the code not used is commented out we can easily hit 95%. Perhaps, we should have more formal and global standard e.g. 90% is acceptable for merging things.
2. rewardEscrow() should have been commented out, agreed. I get what you are saying, maybe for the moment being we can leave it as is till we bring the multi-test here. I don't think it is worth spending more time hitting this line now.

[theethernaut]

@i-stam Ok then, super silly PR that comments out the unused code. It should result in increased test coverage. https://github.com/Synthetixio/synthetix/pull/804

[jjgonecrypto]

Can you also link to: https://github.com/Synthetixio/synthetix/pull/803

We really want these epics to be a summary of all ongoing work