Add drop authorization signing examples and fixtures#179
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughAdds a no-secret drop-authorization signing guide, three deterministic EIP-712/ERC-1271 JSON fixtures, a pure‑Python validator with Keccak/ABI encoders, a unittest harness, CI/Makefile wiring, and updates release manifests, checks, and documentation to validate and publish these artifacts. ChangesDrop Authorization Signing Examples and Fixtures
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_drop_authorization_fixtures.py`:
- Around line 534-561: Add a check that message["poster"] is non-zero for both
sale modes: when sale_mode == 1 and when sale_mode == 2 ensure
message["poster"].lower() != zero_address and raise
DropAuthorizationFixtureError(f"{fixture_id}.poster must be non-zero") if it is
zero; place this validation alongside the existing recipient/payer checks (using
the same fixture_id, zero_address, and error type), so both fixed-price and
auction branches reject a zero poster during message validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 360f0312-2c4e-4ae2-8985-16a4f4a2fef1
📒 Files selected for processing (29)
.github/workflows/ci.ymlCHANGELOG.mdMakefiledocs/audit-package.mddocs/drop-authorization-signing.mddocs/incident-response.mddocs/known-blockers.mddocs/release-readiness.mddocs/tooling.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdrelease-artifacts/latest/SHA256SUMSrelease-artifacts/latest/release-checksums.jsonrelease-artifacts/latest/release-manifest.jsonscripts/check.ps1scripts/check.shscripts/check_audit_package.pyscripts/check_drop_authorization_fixtures.pyscripts/check_incident_response.pyscripts/check_release_readiness.pyscripts/generate_release_checksums.pyscripts/generate_release_manifest.pyscripts/test_drop_authorization_fixtures.pyscripts/test_release_checksums.pyscripts/test_release_manifest.pyscripts/test_release_readiness.pytest/fixtures/drop-authorization/auction-eoa.jsontest/fixtures/drop-authorization/erc1271-contract-signer.jsontest/fixtures/drop-authorization/fixed-price-eoa.json
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/check_drop_authorization_fixtures.py (2)
659-667: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject duplicate
failure_cases[].idvalues inside a fixture.
foundis a set, so repeated ids are silently deduplicated. Combined with the union-only check invalidate_all, a fixture can list the same case twice and still pass if another fixture happens to cover the missing id. Fail fast when an id repeats within the same file.Suggested fix
def validate_failure_cases(fixture: dict[str, Any], fixture_id: str) -> set[str]: """Validate failure case entries and return their IDs.""" cases = require_list(fixture.get("failure_cases"), f"{fixture_id}.failure_cases") found = set() for index, case in enumerate(cases): item = require_dict(case, f"{fixture_id}.failure_cases[{index}]") case_id = require_string(item.get("id"), f"{fixture_id}.failure_cases[{index}].id") require_string(item.get("expected_revert"), f"{fixture_id}.failure_cases[{index}].expected_revert") require_string(item.get("notes"), f"{fixture_id}.failure_cases[{index}].notes") + if case_id in found: + raise DropAuthorizationFixtureError( + f"{fixture_id}.failure_cases[{index}].id duplicates {case_id}" + ) found.add(case_id) return found🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_drop_authorization_fixtures.py` around lines 659 - 667, The current loop silently deduplicates repeated failure case ids because `found` is a set; modify the loop that iterates `cases` (using `require_list`, `require_dict`, `require_string`) to detect and fail on duplicate `case_id` within the same fixture: after computing `case_id` check `if case_id in found:` and raise an error (ValueError or a clear exception) including `fixture_id` and `case_id` so the test fails fast; otherwise add to `found` as before. Ensure `validate_all` behaviour remains unchanged.
620-639: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftValidate the signature bytes, not just their shape.
For
eoa_65_byte, any 65-byte hex blob passes here; forerc1271_mock_bytes, any hex blob paired with the magic value passes. So CI can still publish a fixture whose advertisedexpected.signaturedoes not matchexpected.digestorexpected.signer, even though this checker is supposed to gate deterministic signing examples. Either verify the EOA signature against the digest/signer and pin the ERC-1271 mock bytes, or stop treatingexpected.signatureas validated output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_drop_authorization_fixtures.py` around lines 620 - 639, The checker currently only checks shape of expected.signature (via signature, kind) but not that the signature actually signs expected.digest by expected.signer; update the validation in the block handling kind == "eoa_65_byte" to recover the signer from expected.digest and the 65-byte signature (use the project’s ecrecover utility or implement v/r/s recovery) and compare it to require_string(expected.get("signer"), ...) (raise DropAuthorizationFixtureError on mismatch), and in the "erc1271_mock_bytes" branch pin and validate the mock signature bytes against the expected.erc1271_magic_value (ensure expected.signature equals the agreed mock signature for fixture_id "erc1271-contract-signer" or require a derived value from expected.digest/signer), or alternatively stop treating expected.signature as validated output by explicitly rejecting fixtures that don’t provide a verifiable signature; use symbols require_hex, require_string, DropAuthorizationFixtureError, fixture_id, expected.digest, and expected.signer to locate and implement the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/check_drop_authorization_fixtures.py`:
- Around line 659-667: The current loop silently deduplicates repeated failure
case ids because `found` is a set; modify the loop that iterates `cases` (using
`require_list`, `require_dict`, `require_string`) to detect and fail on
duplicate `case_id` within the same fixture: after computing `case_id` check `if
case_id in found:` and raise an error (ValueError or a clear exception)
including `fixture_id` and `case_id` so the test fails fast; otherwise add to
`found` as before. Ensure `validate_all` behaviour remains unchanged.
- Around line 620-639: The checker currently only checks shape of
expected.signature (via signature, kind) but not that the signature actually
signs expected.digest by expected.signer; update the validation in the block
handling kind == "eoa_65_byte" to recover the signer from expected.digest and
the 65-byte signature (use the project’s ecrecover utility or implement v/r/s
recovery) and compare it to require_string(expected.get("signer"), ...) (raise
DropAuthorizationFixtureError on mismatch), and in the "erc1271_mock_bytes"
branch pin and validate the mock signature bytes against the
expected.erc1271_magic_value (ensure expected.signature equals the agreed mock
signature for fixture_id "erc1271-contract-signer" or require a derived value
from expected.digest/signer), or alternatively stop treating expected.signature
as validated output by explicitly rejecting fixtures that don’t provide a
verifiable signature; use symbols require_hex, require_string,
DropAuthorizationFixtureError, fixture_id, expected.digest, and expected.signer
to locate and implement the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5199ad99-5590-47b5-8de4-cee63890546e
📒 Files selected for processing (3)
ops/AUTONOMOUS_RUN.mdscripts/check_drop_authorization_fixtures.pyscripts/test_drop_authorization_fixtures.py
✅ Files skipped from review due to trivial changes (1)
- ops/AUTONOMOUS_RUN.md
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/test_drop_authorization_fixtures.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
python scripts\test_drop_authorization_fixtures.pypython scripts\check_drop_authorization_fixtures.pypython scripts\test_audit_package.pypython scripts\check_audit_package.pypython scripts\test_incident_response.pypython scripts\check_incident_response.pypython scripts\test_release_readiness.pypython scripts\check_release_readiness.pypython scripts\test_release_manifest.pypython scripts\generate_release_manifest.py --checkpython scripts\test_release_checksums.pypython scripts\generate_release_checksums.py --checkgit diff --checkmake checkCloses #177
Summary by CodeRabbit
New Features
Documentation
Tests
Chores