feat: update claim_contract README and Makefile#2286
Conversation
Codex Code ReviewFound one issue:
No security vulnerabilities or significant performance issues found in this diff. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| --rpc-url $(PROD_RPC_URL) \ | ||
| --rpc-url $(MAINNET_RPC_URL) \ | ||
| --broadcast \ | ||
| --verbosity 3 |
There was a problem hiding this comment.
Low — deploy-token-mainnet is missing --verify --etherscan-api-key $(ETHERSCAN_API_KEY), unlike every other network-deployment target (deploy-token-sepolia, deploy-claimable-sepolia, deploy-claimable-base-sepolia, deploy-claimable-mainnet). An unverified mainnet token contract is hard to audit after the fact.
| --verbosity 3 | |
| deploy-token-mainnet: ## 🚀 Deploy the token contract in Mainnet | |
| cd script && \ | |
| forge script DeployAlignedToken.s.sol \ | |
| --sig "run(string)" \ | |
| mainnet \ | |
| --keystore $(KEYSTORE_PATH) \ | |
| --rpc-url $(MAINNET_RPC_URL) \ | |
| --broadcast \ | |
| --verify \ | |
| --etherscan-api-key $(ETHERSCAN_API_KEY) \ | |
| --verbosity 3 |
| AMOUNT_TO_CLAIM = $(shell curl -S -H "Content-Type: application/json" http://localhost:4000/api/proof/\$(CLAIMER) | jq -r .amount) | ||
| MERKLE_PROOF_TO_CLAIM = $(shell curl -S -H "Content-Type: application/json" http://localhost:4000/api/proof/\$(CLAIMER) | jq .proof | tr -d '"\n ') |
There was a problem hiding this comment.
Low — Both variables hit the same URL but as separate $(shell ...) calls. With = (recursive expansion), Make re-runs each shell command every time the variable is referenced, so test-claim makes two separate HTTP requests to the proof API for the same CLAIMER. A single call that captures both fields would be more reliable.
Additionally, note these variables are already flagged via the test-claim TODO comment as outdated (wrong ABI, wrong endpoint). Consider removing them along with test-claim instead of moving them to the top-level.
Review: feat: update claim_contract README and MakefileGood 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] [Low] cast calldata "approve(address,uint256)" $(CLAIM_PROXY_ADDRESS) 2600000000000000000000000000The direct-send counterpart ( Code quality[Low] Broken targets should be removed, not annotated |
Codex Code Review
|
| test-claim: | ||
| cast send $(AIRDROP) --private-key $(CLAIMER_PRIVATE_KEY) "claim(uint256,bytes32[])" $(AMOUNT_TO_CLAIM) "$(MERKLE_PROOF_TO_CLAIM)" --rpc-url $(RPC_URL) | ||
|
|
||
| # TODO: outdated — hasClaimed is now hasClaimed(bytes32 leaf), not hasClaimed(address). Fix. |
There was a problem hiding this comment.
Bug (Low/Medium): test-claim/test-claimed are shipped broken, per your own TODOs — claim(uint256,bytes32[]) is missing the validFrom param the current contract expects, and hasClaimed(address) doesn't match hasClaimed(bytes32). Since these are dev/debug targets, consider either fixing them now (cheap) or removing them instead of leaving dead, misleading tooling that will silently fail (or worse, succeed against the wrong overload) if someone runs make test-claim.
| # TODO: broken — reads script-out/deployed_token_addresses.json (never generated by the deploy | ||
| # scripts) and treats $(CONFIG) as a file path. Fix or remove. | ||
| update_token_proxy: | ||
| @NEW_TOKEN_PROXY=$$(jq -r '.tokenProxy' "script-out/deployed_token_addresses.json") && \ | ||
| jq --arg new_proxy "$$NEW_TOKEN_PROXY" '.tokenProxy = $$new_proxy' $(CONFIG) > $(CONFIG).tmp \ | ||
| && mv $(CONFIG).tmp $(CONFIG) | ||
|
|
||
| # TODO: broken — runs UpgradeToken.s.sol, which does not exist in script/. Fix or remove |
There was a problem hiding this comment.
Simplicity: update_token_proxy, upgrade-token, and upgrade-aligned-token-implementation (line 200) all reference scripts that don't exist in script/. Marking them broken with a TODO is better than silence, but a Makefile is meant to be run — dead targets left in .PHONY and help output invite someone to invoke them and hit confusing failures. Consider just deleting them (git history preserves them) until the scripts actually exist, rather than shipping known-broken automation.
| AMOUNT_TO_CLAIM = $(shell curl -S -H "Content-Type: application/json" http://localhost:4000/api/proof/\$(CLAIMER) | jq -r .amount) | ||
| MERKLE_PROOF_TO_CLAIM = $(shell curl -S -H "Content-Type: application/json" http://localhost:4000/api/proof/\$(CLAIMER) | jq .proof | tr -d '"\n ') |
There was a problem hiding this comment.
Minor: AMOUNT_TO_CLAIM and MERKLE_PROOF_TO_CLAIM each hit /api/proof/<claimer> separately. Since both are only used together in test-claim, a single curl call piped through jq for both fields (or a one-shot shell recipe in test-claim itself) would avoid the duplicate network round-trip. Not a big deal for a local dev target, just a simplification.
Review summaryThis PR only touches Overall: solid documentation and Makefile ergonomics cleanup — clearer network-scoped RPC/key variables ( Findings (left as inline comments):
No security vulnerabilities identified (default private keys used are the well-known public Anvil/Foundry test-account keys, not real secrets). |
2 participants
update claim_contract README and Makefile
Description
Checklist
testnet, everything else tostaging