chore: IdRegistry + KeyRegistry review

horsefacts commented 1 year ago

Change Summary

Clean up comments and natspec. Use simple expectEmit(), which checks all indexed params and event data.

Merge Checklist

Choose all relevant options below by adding an x now or at any time before submitting for review

[x] The PR title adheres to the conventional commits standard
[x] The PR has been tagged with change type label(s) (i.e. documentation, feature, bugfix, or chore)
[x] The PR's changes adhere to all the requirements in the contribution guidelines
[x] This PR does not require changes to the Farcaster protocol

Additional Context

If this is a relatively large or complex change, provide more details here that will help reviewers.

PR-Codex overview

This PR focuses on improving the clarity and accuracy of code comments and function parameter names.

Detailed summary

Renamed _owner parameter to _initialOwner in TrustedCaller and FnameResolver constructors.
Updated comments in StorageRegistry and KeyRegistry functions to improve clarity.
Renamed owner parameter to _initialOwner in Bundler constructor.
Renamed owner parameter to _initialOwner in KeyRegistry constructor.
Updated comments in KeyRegistry functions to improve clarity.

The following files were skipped due to too many changes: src/KeyRegistry.sol, src/IdRegistry.sol

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

github-actions[bot] commented 1 year ago

Coverage after merging horsefacts/ir-kr-review into main will be

98.75%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
src
Bundler.sol	100%	100%	100%	100%
FnameResolver.sol	100%	100%	100%	100%
IdRegistry.sol	100%	100%	100%	100%
KeyRegistry.sol	100%	100%	100%	100%
StorageRegistry.sol	100%	100%	100%	100%
src/lib
Signatures.sol	100%	100%	100%	100%
TransferHelper.sol	0%	0%	0%	0%	12, 17, 20, 20, 20
TrustedCaller.sol	100%	100%	100%	100%

horsefacts commented 1 year ago

Two things to consider, but I don't think either need immediate fixes.

1) The fact that FID transfers preserve the registered recovery address could be unexpected behavior for some users. This is by design, but could be a surprise if e.g. someone sells their FID to another address but the recovery settings persist. I think this is OK, since a FID doesn't really have much value separate from an associated name, but we should make this behavior clear to end users. (In fact, doing so probably discourages selling FIDs).

2) I don't like that it's possible to register an invalid or malformed key in the key registry, but there's not really a way to validate against every scheme. Again, this is more of a sharp point to warn about: consumers of the key registry should know that users can register whatever bytes they want as a key and it's up to the caller to validate.

varunsrin commented 1 year ago

Good callouts, some thoughts:

The original design used to reset the recovery but users may not expect this and forget to configure recovery again after they recover. Since we don't really want people to be trading fids, accepting a little badness in that case for better protection against users seems like the right tradeoff.
Agree, I wish we could validate but will become impractical if we want to make it extensible in the future.

farcasterxyz / contracts