Skip to content

Conversation

@gravityblast
Copy link
Member

This PR implements issue(s) #83 #81 #80

Checklist

  • check that the slashed account is the same of the one used in the commitment phase
  • fix doc comment
  • use the slasher address to compute the slash commitment id
  • make the slash function internal to avoid front-running

@gravityblast gravityblast marked this pull request as draft December 4, 2025 15:36
karma.slash(member.userAddress, rewardRecipient);
delete members[identityCommitment];

emit MemberSlashed(member.index, msg.sender);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for this please?

@gravityblast gravityblast marked this pull request as ready for review December 5, 2025 11:24
delete members[identityCommitment];

emit MemberSlashed(member.index, msg.sender);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change. Would you mind updating the commit accordingly with a breaking change section, as per https://www.conventionalcommits.org/en/v1.0.0/#examples

An example can also be found here: #85

bytes32 hash = keccak256(abi.encodePacked(privateKey, rewardRecipient));
uint256 revealStartTime = slashCommitments[account][hash];
bytes32 key = _slashCommitmentKey(msg.sender, hash);
uint256 revealStartTime = slashCommitments[account][key];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a fix, we should make it a fix commit. ideally it references the issue it closes

/// @dev Sets the slash reveal window time.
/// @param _slashRevealWindowTime: new reveal window time in seconds.
/// @notice The window time must be at least 1 second and no more than 365 days.
/// @notice The window time must be at least 1 second and no more than 1 day.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reference the issue that it will close?

Copy link
Member

@0x-r4bbit 0x-r4bbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM!

Only thing I'd adjust is that every commit references the issue they fix.

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