OpenZeppelin / OpenZeppelin/compact-contracts

Followup: `FungibleToken refactor -> replace pk with sk #468`

Open
#497 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

1-medium effort: 2-S feat:token
Dominant language
TypeScript
Stars
55
Forks
29
Avg merge
5d 7h
Merged PRs (30d)
25

Description

Some buckets overlap with #496 — flagged below so we don't duplicate work.

Generalize helpers into Utils.compact (overlaps with #496)

The isContractAddress left-branch check is now repeated across four
public circuits, and a Bytes<32> default-Left helper is duplicated
between FungibleToken and Ownable in different shapes.

  • Generalize the isContractAddress left branch. Call sites:
    L244,
    L364,
    L434,
    L538.
  • Move the local ZERO() / default-Either helper into Utils.compact
    (the Ownable PR has a verbose variant of the same thing).
    Refs: r3167100097,
    L721.
Shared AccountId module (overlaps with #496)

_computeAccountId lives in FungibleToken today; it should be a shared
module reused by Ownable, AccessControl, and any future identity-bearing
contract.
Ref: L710.

Redundant defensive guards on internal call chains

Initializable_assertInitialized and Utils_canonicalize re-fire at
every layer of the public-circuit call graph:

  • transfer -> assertInit fires 4x (transfer -> _unsafeTransfer -> _unsafeUncheckedTransfer -> _update).
  • transferFrom -> assertInit fires 6x (transferFrom -> _unsafeTransferFrom -> _spendAllowance -> _approve -> _unsafeUncheckedTransfer -> _update).
  • Same pattern in AccessControl and Ownable.

Author agreed to re-evaluate; the current "defensive > optimized" stance
may belong at the language layer instead. Decide and either prune or
document.
Refs: r3167195733,
r3170298119.

Witness module polish
  • Make the witness file header say explicitly that this witness is
    for tests / node use, not the production identity flow.
    Ref: r3167084797.
  • Unify nonce vs key terminology (mirror item from #496).
    Ref: r3167088237.
  • PrivateState docstring nit: "Represents the private state of a FungibleToken contract, storing a secret key."
    Ref: r3167147400.
Syntax
  • ZERO() export should be pure: export pure circuit ZERO(): Either<Bytes<32>, ContractAddress>.
    Ref: r3167128644.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the four isContractAddress call sites and the duplicated ZERO/default-Either helpers referenced from PR #468, then compare the overlapping scope in #496. Review the FungibleToken, Ownable, and AccessControl call chains and the witness file for the listed refactors, guard decisions, terminology, and documentation. Done means the overlapping helpers and AccountId logic are resolved, redundant guards are pruned or documented, and every checklist item is addressed.

Written by the indexing model from the issue text.

Assessment

Domain
blockchain
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.