Skip to content
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

Implement MIP-74 #992

Open
wants to merge 6 commits into
base: feature/trusted-relayer
Choose a base branch
from
Open

Implement MIP-74 #992

wants to merge 6 commits into from

Conversation

Primata
Copy link
Contributor

@Primata Primata commented Jan 9, 2025

Summary

  • RFCs: Link to RFC, Link to RFC, or $\emptyset$.
  • Categories: any of protocol-units, networks, scripts, util, cicd, or misc.

Implements MIP-74 specs

Changelog

adds outbound rate limit
adds PAUSER_ROLE

Testing

Outstanding issues

@Primata Primata changed the title Implement-74 Implement MIP-74 Jan 9, 2025
@Primata Primata requested a review from apenzk January 9, 2025 22:54
bytes32 public constant PAUSER_ROLE = keccak256(abi.encodePacked("PAUSER_ROLE"));

// Risk denominator must be above 3
uint256 public constant RISK_DENOMINATOR_LOWER_BOUND = 3;
Copy link

@apenzk apenzk Jan 10, 2025

Choose a reason for hiding this comment

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

lower bound implies 3 is included, but you state "must be above 3", hence i modify to 4 below

uint256 public constant MINIMUM_RISK_DENOMINATOR = 3;
bytes32 public constant PAUSER_ROLE = keccak256(abi.encodePacked("PAUSER_ROLE"));

// Risk denominator must be above 3
Copy link

@apenzk apenzk Jan 10, 2025

Choose a reason for hiding this comment

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

Suggested change
// Risk denominator must be above 3
// The riskDenominator determines the fraction of the insurance fund that can be used for the per day budget for a given transfer direction.
// Risk denominator must be equal or above 4

Copy link

@apenzk apenzk left a comment

Choose a reason for hiding this comment

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

looks good for an initial approach apart from

  • in the sol contract the risk_denominator cannot be updated. I am ok with this but it is inconsistent with the move contract equivalent
  • requesting a few minor changes for acceptance.

long term, with more complex approaches like sliding window, we should consider the length of the window, but for now setting the window length to 1day is a good minimal approach.

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