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

MarketUpdated event #1697

Merged
merged 10 commits into from
Jul 5, 2023
Merged

MarketUpdated event #1697

merged 10 commits into from
Jul 5, 2023

Conversation

0xjocke
Copy link
Contributor

@0xjocke 0xjocke commented Jun 30, 2023

Include market skew and market size in the settle order event

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #1697 (0f26123) into main (9fa0cde) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1697   +/-   ##
=======================================
  Coverage   72.77%   72.77%           
=======================================
  Files          57       57           
  Lines         720      720           
  Branches      236      236           
=======================================
  Hits          524      524           
  Misses        167      167           
  Partials       29       29           
Flag Coverage Δ
core-contracts 93.26% <ø> (ø)
core-modules 90.47% <ø> (ø)
core-utils 68.57% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Note that for an event to be emitted from a library, it needs to be defined in two places:
- The library itself, and
- The interface of the contract that is using the library.
@0xjocke
Copy link
Contributor Author

0xjocke commented Jul 4, 2023

A bit of a journey but I think I got there in the end.
Added

    event MarketUpdated(
        uint128 marketId,
        int256 skew,
        uint256 size,
        int256 sizeDelta,
        int256 currentFundingRate,
        int256 currentFundingVelocity
    );

Learned that if you want to emit an event from a storage library you need to define it:

  1. In the library
  2. In the contract using it. (or that contract's interface)

Doing this we pushed the AsyncOrderModule over the size limit, so I created a new AsyncOrderViewModule

@0xjocke 0xjocke requested a review from Tburm July 4, 2023 05:44
@0xjocke 0xjocke changed the title Settle event improvement MarketUpdated event Jul 4, 2023
Copy link
Contributor

@leomassazza leomassazza left a comment

Choose a reason for hiding this comment

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

LGTM

markets/perps-market/contracts/storage/PerpsMarket.sol Outdated Show resolved Hide resolved
markets/perps-market/contracts/storage/PerpsMarket.sol Outdated Show resolved Hide resolved
const pSkew = wei(skew).div(skewScale);
const pSkewBounded = Wei.max(Wei.min(pSkew, wei(1)), wei(-1));

return pSkewBounded.mul(maxFundingVelocity);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe returns bignumber via .toBN() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coming from the frontend we usually keep everything in Wei and just toBN before sending to ethers contract call. Wei usually makes math and most things easier.
But we do seem to like/use BigNumber more in contract tests, so I guess it make sense to return bn.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok cool yeah fair enough!

Copy link
Contributor

@sunnyvempati sunnyvempati left a comment

Choose a reason for hiding this comment

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

awesome! tyty

//SPDX-License-Identifier: MIT
pragma solidity >=0.8.11 <0.9.0;

interface IMarketEvents {
Copy link
Contributor

Choose a reason for hiding this comment

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

any others around that we can throw in here maybe? 👀

Copy link
Contributor Author

@0xjocke 0xjocke Jul 5, 2023

Choose a reason for hiding this comment

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

Most modules will have their events defined in their own interface. I had a look, didn't see any clear ones to move

@@ -131,6 +131,7 @@ library AsyncOrder {
uint initialRequiredMargin;
uint totalRequiredMargin;
Position.Data newPosition;
bytes32 trackingCode;
Copy link
Contributor

Choose a reason for hiding this comment

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

still need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, without it we get "Stack too deep." Just at the limit now 😓

@sunnyvempati
Copy link
Contributor

hmm some tests failed; seems legitimate?

@0xjocke
Copy link
Contributor Author

0xjocke commented Jul 5, 2023

hmm some tests failed; seems legitimate?

It does looks like that.. Looking now, pretty sure I ran it locally...

@0xjocke
Copy link
Contributor Author

0xjocke commented Jul 5, 2023

hmm some tests failed; seems legitimate?

It does looks like that.. Looking now, pretty sure I ran it locally...

Was a fluke :(... Reran fixed it

@0xjocke 0xjocke merged commit df26ebd into main Jul 5, 2023
19 checks passed
@0xjocke 0xjocke deleted the settle-event-improvement branch July 5, 2023 02:42
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.

4 participants