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

improve(funding-rate-applier): Replace NewFundingRateMultiplier event with FundingRateUpdated #2284

Merged
merged 1 commit into from
Dec 10, 2020

Conversation

nicholaspai
Copy link
Member

@nicholaspai nicholaspai commented Dec 9, 2020

Signed-off-by: Nick Pai npai.nyc@gmail.com

Motivation

#2282

Issue(s)

Fixes #2282

Signed-off-by: Nick Pai <npai.nyc@gmail.com>
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 93.216% when pulling 07ab79f on npai/2282 into 47e1e75 on master.

@nicholaspai nicholaspai marked this pull request as ready for review December 10, 2020 13:04
int256 indexed latestFundingRate,
int256 periodRate
);
event FundingRateUpdated(int256 newFundingRate, uint256 indexed updateTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should also have an event which emits the new cumulative multiplier. I cant really forsee how that would be useful though. I imagine if some offchain infra was using events to recreate historical contract state maybe this one event is enough to do that. thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think its sufficient to just read the cumulative multiplier on-chain. I don't think there's a need to see historical multipliers, but you should always know the current multiplier before modifying a position.

Copy link
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

This pull request was closed.
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.

Remove NewFundingRateMultiplier event and replace with FundingRateUpdated
4 participants