openzeppelin-upgrades icon indicating copy to clipboard operation
openzeppelin-upgrades copied to clipboard

Code for the rename catch is confusing.

Open Amxx opened this issue 3 years ago • 2 comments

This code is not really clear

    const nameChange =
      !this.unsafeAllowRenames &&
      original.label !== updated.renamedFrom &&
      (updated.label !== original.label ||
        (updated.renamedFrom !== undefined && updated.renamedFrom !== original.renamedFrom));

(updated.renamedFrom !== undefined && updated.renamedFrom !== original.renamedFrom) is "if we are renaming, and we are not renaming what original renames from".

Amxx avatar Aug 29 '22 14:08 Amxx

This is slower to run, but is IMO way clearer:

const nameChange =
    !this.unsafeAllowRenames &&
    (
        // the original label is neither the new label, not the renamedFrom
        ![ updated.label, updated.renamedFrom ].includes(original.label) ||
        // the renamedFrom is set, and doesn't match either the previous label, or the previous rename
        ![ undefined, original.label, original.renamedFrom ].includes(updated.renamedFrom)
    )

Amxx avatar Aug 29 '22 14:08 Amxx

The suggested code looks good to me. Can you open a PR?

frangio avatar Aug 29 '22 14:08 frangio