Skip to content

feat: update claim_contract README and Makefile#2286

Merged
JuArce merged 3 commits into
testnetfrom
update_claim_contracts_with_base
Jul 22, 2026
Merged

feat: update claim_contract README and Makefile#2286
JuArce merged 3 commits into
testnetfrom
update_claim_contracts_with_base

Conversation

@JuArce

@JuArce JuArce commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

update claim_contract README and Makefile

Description

Checklist

  • “Hotfix” to testnet, everything else to staging
  • Linked to Github Issue
  • This change depends on code or research by an external entity
    • Acknowledgements were updated to give credit
  • Unit tests added
  • This change requires new documentation.
    • Documentation has been added/updated.
  • This change is an Optimization
    • Benchmarks added/run
  • Has a known issue
  • If your PR changes the Operator compatibility (Ex: Upgrade prover versions)
    • This PR adds compatibility for operator for both versions and do not change crates/docs/examples
    • This PR updates batcher and docs/examples to the newer version. This requires the operator are already updated to be compatible

@JuArce JuArce self-assigned this Jun 19, 2026
@github-actions

Copy link
Copy Markdown

Codex Code Review

Found one issue:

  • Low - Testnet deploy instructions reference config files that are not added.
    claim_contracts/README.md tells users to fill script-config/config.sepolia.json and config.base-sepolia.json, and the new Make targets pass sepolia / base-sepolia to scripts that read script-config/config.<network>.json. This PR only adds config.sepolia.example.json and config.base-sepolia.example.json, so a fresh checkout following the docs will fail at vm.readFile(...) unless the user guesses they must copy the example files. Add the real empty config files, or make the docs explicitly say to copy the examples first.

No security vulnerabilities or significant performance issues found in this diff.

Comment thread claim_contracts/Makefile
Comment thread claim_contracts/Makefile
@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review: feat: update claim_contract README and Makefile

Good overall — the README rewrite is much clearer, the config consolidation is the right call, and the TODO annotations on broken targets are honest. Two real bugs and one inconsistency to address.

Bugs

[Low] deploy-token-mainnet is missing --verify (inline comment)
Every other network-deploy target verifies the contract on Etherscan; mainnet token deployment is the odd one out. Unverified mainnet contracts undermine auditability.

[Low] calldata-approve-spending hardcodes the approve amount (line 52, pre-existing but worth fixing here)

cast calldata "approve(address,uint256)" $(CLAIM_PROXY_ADDRESS) 2600000000000000000000000000

The direct-send counterpart (approve-claimable) uses $(APPROVE_AMOUNT), so overriding APPROVE_AMOUNT= on the command line is silently ignored for the calldata path. Change to $(APPROVE_AMOUNT) for consistency.

Code quality

[Low] Broken targets should be removed, not annotated
update_token_proxy, upgrade-token, upgrade-aligned-token-implementation, test-claim, and test-claimed are all documented as broken via TODO comments. Leaving them in .PHONY and callable means a developer can accidentally run them and get a misleading failure. Removing them (or at minimum dropping them from .PHONY and adding a @echo "broken" guard) is safer than annotating.

@JuArce
JuArce marked this pull request as ready for review July 14, 2026 13:08
@github-actions

Copy link
Copy Markdown

Codex Code Review

  • Medium – enable-claimability does not enforce transaction ordering. Makefile models the four transactions as independent prerequisites. With make -j or global MAKEFLAGS, they run concurrently, causing nonce collisions and potentially attempting unpause before the root/deadline updates. This contradicts the documented ordering. Chain the targets explicitly or execute them sequentially in one recipe.

Comment thread claim_contracts/Makefile
Comment thread claim_contracts/Makefile
Comment thread claim_contracts/Makefile
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review summary

This PR only touches claim_contracts/Makefile, README.md, and two new example config JSONs — no Solidity/Rust/API code changes, so no reentrancy/access-control/unsafe-code type risks apply here.

Overall: solid documentation and Makefile ergonomics cleanup — clearer network-scoped RPC/key variables (SEPOLIA_RPC_URL, MAINNET_RPC_URL, BASE_SEPOLIA_RPC_URL), a consolidated enable-claimability composite target, and a much clearer README flow (deploy → enable claiming per network). The new config.sepolia.example.json / config.base-sepolia.example.json token addresses match what's documented in the README.

Findings (left as inline comments):

  • Low/Medium — test-claim/test-claimed targets are broken against the current contract ABI (per the PR's own added TODOs) and were left in rather than fixed or removed.
  • Low — update_token_proxy, upgrade-token, upgrade-aligned-token-implementation reference scripts that don't exist; consider deleting dead targets instead of leaving them (with TODOs) in .PHONY/help.
  • Nit — AMOUNT_TO_CLAIM/MERKLE_PROOF_TO_CLAIM make two separate calls to the same proof endpoint.

No security vulnerabilities identified (default private keys used are the well-known public Anvil/Foundry test-account keys, not real secrets).

@JuArce
JuArce merged commit 72eee1e into testnet Jul 22, 2026
2 checks passed
@JuArce
JuArce deleted the update_claim_contracts_with_base branch July 22, 2026 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants