Conversation
|
coverage and linting workflows are outdated |
There was a problem hiding this comment.
Pull Request Overview
This PR implements an invite system for Safety Net contracts, allowing owners to create off-chain signed invites for progressive member onboarding. The implementation uses EIP-712 signatures to maintain privacy while preventing replay attacks through nonces.
Key changes:
- Added EIP-712 based invite struct with signature verification
- Implemented single-use nonce system to prevent invite reuse
- Added comprehensive error handling for invite redemption edge cases
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/interfaces/ISafetyNet.sol | Defines Invite struct, InviteRedeemed event, invite-related errors, and redeemInvite function interface |
| src/contracts/SafetyNet.sol | Implements EIP-712 signature verification, nonce tracking, and invite redemption logic with proper validations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/contracts/SafetyNet.sol
Outdated
| if (usedNonces[_invite.safetyNetId][_invite.nonce]) revert InviteAlreadyUsed(); | ||
| if (isMember[_invite.safetyNetId][msg.sender]) revert AlreadyMember(); | ||
| if (_safetyNet.members.length >= _safetyNet.maximumMembers) revert SafetyNetFull(); | ||
| if (_invite.redeemer != address(0) && _invite.redeemer != msg.sender) revert NotRedeemer(); |
There was a problem hiding this comment.
The logic assumes that address(0) means 'anyone can redeem', but this behavior is not documented in the struct definition or function comments. Consider adding documentation to clarify that redeemer = address(0) creates an open invite.
| ) | ||
| ); | ||
|
|
||
| return keccak256(abi.encodePacked('\x19\x01', _domainSeparator, _structHash)); |
There was a problem hiding this comment.
[nitpick] Consider using abi.encode instead of abi.encodePacked for the final hash construction. While this follows EIP-712 specification correctly, abi.encode is generally safer and the gas difference is negligible for this use case.
| return keccak256(abi.encodePacked('\x19\x01', _domainSeparator, _structHash)); | |
| return keccak256(abi.encode('\x19\x01', _domainSeparator, _structHash)); |
|
@exo404 i think we should take the redeemer off scope completely. it was not in the spec, not sure why it was implemented. the whole point of an invite link is to not specify an address, this is the entire pain point we are trying to solve with this feature. |
Acrually if you write 0x0 address the invite is open. I may read wrong but you and bagel said you wanted option so this is in the spec too. I can change it tho that's not a problem @RonTuretzky @bagelface |
It's not in the UML. I don't think we need it at all. Would prefer to not have it. |
I forgot to specify the 0x0 thing you right. I'll change the code and the spec |
|
|
please add something like this and this to this PR. eventually we will need to do a typescript version as well but we can open a seperate issue for this. cc @crystal503 |
|
@RonTuretzky working on this now |
|
RonTuretzky
left a comment
There was a problem hiding this comment.
please add test for the invite validation
script/InviteGenerator.sol
Outdated
| /// @notice Utility contract that produces Safety Net invite signatures matching the on-chain verification logic | ||
| contract InviteGenerator is Script { | ||
| /// @notice Invite signing domain name used for EIP-712 signatures | ||
| string private constant INVITE_SIGNING_DOMAIN = 'SafetyNetInvite'; |
There was a problem hiding this comment.
getting warnings on the next 3 variables they should start with _
script/README_INVITES.md
Outdated
There was a problem hiding this comment.
add to the README instead of introducing another file
There was a problem hiding this comment.
please have some discretion, this file seems very long
|
|
@RonTuretzky done with testing, both for SafetyNet redeemInvite function and InviteGenerator contract |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
script/InviteGenerator.sol
Outdated
| } | ||
|
|
||
| /// @notice Computes the full EIP-712 digest for a struct invite | ||
| function inviteDigest( |
There was a problem hiding this comment.
Use abi.encodePacked instead of abi.encode on line 70 for the EIP-712 digest. According to EIP-712 specification, the final digest should use encodePacked for the \x19\x01 prefix concatenation.
test/unit/InviteGeneratorUnit.t.sol
Outdated
| function test_shouldReturnsInviteDigest() public { | ||
| bytes32 structHash = inviteGenerator.hashInvite(STRUCT_ID, NONCE); | ||
| bytes32 domainSeparator = inviteGenerator.domainSeparator(CHAIN_ID, VERIFYING_CONTRACT); | ||
| bytes32 expectedDigest = keccak256(abi.encode('\x19\x01', domainSeparator, structHash)); |
There was a problem hiding this comment.
Use abi.encodePacked instead of abi.encode for the EIP-712 digest in the test expectation to match the EIP-712 specification.
| bytes32 expectedDigest = keccak256(abi.encode('\x19\x01', domainSeparator, structHash)); | |
| bytes32 expectedDigest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, structHash)); |
RonTuretzky
left a comment
There was a problem hiding this comment.
here is a refrerene for a cfrontend compoenent that is compatible with a less rigorus implementation , please adjust to work with this implementation , test locally , it is enough just to document a correct typescript/javacript snippet that works with this solidity code
script/InviteGenerator.sol
Outdated
| /// @notice Utility contract that produces EIP712 invite signatures matching the on-chain verification logic | ||
| contract InviteGenerator is Script { | ||
| /// @notice Invite signing domain name used for EIP-712 signatures | ||
| string private _inviteSigningDomain; |
There was a problem hiding this comment.
drop in favor of a comment on the hasehd version
script/InviteGenerator.sol
Outdated
| string private _inviteSigningDomain; | ||
|
|
||
| /// @notice Invite signing version used for EIP-712 signatures | ||
| string private _inviteSignatureVersion; |
There was a problem hiding this comment.
drop string in favor of comment next to hashed version
|
@RonTuretzky all test pass, I'm merging this to start to work on the frontend component locally |
Technical Spec – Safety Net Invite
1. Background
Problem Statement:
Today, when creating a Safety Net (SafetyNet), all members must be declared upfront. This makes progressive onboarding difficult and slows down organizers. The first design with on-chain stored invites exposed them publicly. A later proposal with counters and Merkle roots was too complex for the core use case.
Context / History:
Stakeholders:
2. Motivation
Goals & Success Stories:
nonce.3. Scope and Approaches
Non-Goals:
Value Proposition:
Alternative Approaches:
Relevant Metrics:
4. Step-by-Step Flow
4.1 Main (“Happy”) Path
fundId, nonceand signs with EIP-712.msg.senderas member.InviteRedeemed.4.2 Alternate / Error Paths
revert Invite usedrevert Invalid signerrevert Already member5. UML Diagrams
Class Diagram
classDiagram class SafetyNet { +redeemInvite(Invite inv, bytes sig) -usedNonces[fundId][nonce]: bool -isMember[fundId][addr]: bool -ownerOf[fundId]: address } class Invite { +fundId: uint256 +nonce: uint256 } SafetyNet o--> Invite : verifiesSequence Diagram
sequenceDiagram participant Owner participant User participant Safety Net Contract Owner->>Owner: Sign Invite {fundId, nonce} Owner->>User: Share link (Invite+sig) User->>Safety Net Contract: redeemInvite(inv, sig) Safety Net Contract->>Safety Net Contract: validate signature, nonce, and that the caller isn't already a member Safety Net Contract->>Safety Net Contract: mark nonce used, add member Safety Net Contract-->>User: InviteRedeemed eventState Diagram
stateDiagram [*] --> Valid Valid --> Used : nonce consumed5. Edge cases and concessions
6. Open Questions
7. Glossary / References
(fundId, nonce).Links: