Skip to content

Implement VotingStreakMultiplier #129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 21 commits into
base: dev
Choose a base branch
from
Open

Implement VotingStreakMultiplier #129

wants to merge 21 commits into from

Conversation

secbajor
Copy link
Collaborator

@secbajor secbajor commented Apr 24, 2025

When a vote is cast (via the YieldDistributor), the VotingStreakMultiplier updates it's state variables tracking user's multiplier value and validity.

When this contract is deployed, the front end can call getMultiplyingFactor for the user's current multiplier.

@secbajor secbajor changed the title Sb 127 2 Implement VotingStreakMultiplier Apr 24, 2025
Copy link
Contributor

@bagelface bagelface left a comment

Choose a reason for hiding this comment

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

needs slight redesign

Copy link
Collaborator

@RonTuretzky RonTuretzky left a comment

Choose a reason for hiding this comment

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

pattern is implemented as expected! looking very elegant!

@secbajor
Copy link
Collaborator Author

secbajor commented May 8, 2025

@bagelface @RonTuretzky with my latest commit here, the build is compiling without errors/warnings and the tests all pass. Can you advise on next steps?

Copy link
Collaborator

@RonTuretzky RonTuretzky left a comment

Choose a reason for hiding this comment

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

LGTM so far , please add a VotingStreakMultiplier.t.sol file with unit tests

Comment on lines +99 to +105
/// @notice This is a no-op function that exists solely to implement the IMultiplier interface
/// @dev This function does nothing and always returns, as the actual multiplier update logic
/// is handled by the parameterless updateMultiplyingFactor() function
/// @param _newMultiplyingFactor Unused parameter
function updateMultiplyingFactor(uint256 _newMultiplyingFactor) external pure override {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can remove this one from this contract and the upstream interfaces as well if we're not using it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i thought this was a function to be called externally in case we ever decide to update the multiplier value. like, if we start out thinking discord membership should give 1.05x but then later 1.1x or something, would this be the way to make that update?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i confused myself a bit but that's my thinking for why we need to keep it in the upstream interfaces, since those are implemented by other kinds of multipliers. and so my understanding is it has to be implemented here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think basically you added one that was meant to be for this pr , then we figured out we need that one to accept address not an int, and now we have the previous one that's still there that we need to remove , no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't see where the int version of updateMultiplyingFactor is being used so bumping this for its removal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@RonTuretzky the idea seems to be that it will be used by NFTMultiplier here

Copy link
Collaborator

Choose a reason for hiding this comment

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

why would it not just be calculated and called internally? it being supplied implies that some outside contract is doing the logic to determine the multiplier update, but this is an anti pattern to the system design, as all multiplier logic is meant to be self contained within the multiplier contracts themselves

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.

3 participants