farcasterxyz / contracts

Implementation of the Farcaster contracts
https://www.farcaster.xyz/
MIT License
340 stars 122 forks source link

test(StorageRegistry): add docs and tests to improve state coverage #314

Closed varunsrin closed 1 year ago

varunsrin commented 1 year ago

Motivation

Cleaning up comments and improving test coverage.

Change Summary

Cleaning up comments and improving test coverage.

Merge Checklist


PR-Codex overview

Detailed summary

The following files were skipped due to too many changes: src/StorageRegistry.sol

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

github-actions[bot] commented 1 year ago

Coverage after merging varunsrin/storage-registry-review-2 into main will be

98.75%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   Bundler.sol100%100%100%100%
   FnameResolver.sol100%100%100%100%
   IdRegistry.sol100%100%100%100%
   KeyRegistry.sol100%100%100%100%
   StorageRegistry.sol100%100%100%100%
src/lib
   Signatures.sol100%100%100%100%
   TransferHelper.sol0%0%0%0%12, 17, 20, 20, 20
   TrustedCaller.sol100%100%100%100%
varunsrin commented 1 year ago

@horsefacts couple of things we could add in (but wanted to run by you):

  1. Should we hardcode the _initialDeprecationPeriod in the constructor to 1 year? I'm pretty sure we'll want to deploy with that default for now, so we can eliminate a configuration param.
  2. We can probably use unchecked in a couple of places to reduce a little gas, but is it worth it given your testing of L2 gas usage?
horsefacts commented 1 year ago
  • Should we hardcode the _initialDeprecationPeriod in the constructor to 1 year? I'm pretty sure we'll want to deploy with that default for now, so we can eliminate a configuration param.

Yeah, I'll make this refactor when I get a sec.

  • We can probably use unchecked in a couple of places to reduce a little gas, but is it worth it given your testing of L2 gas usage?

Nah, probably not worth it at this point, based on our L2 profiling.