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

Update OrderSettled event #1696

Closed
wants to merge 4 commits into from
Closed

Update OrderSettled event #1696

wants to merge 4 commits into from

Conversation

Tburm
Copy link
Contributor

@Tburm Tburm commented Jun 29, 2023

  • Add sizeDelta to the OrderSettled event - This will help calculate volume (sizeDelta * fillPrice)
  • Fix a typo in the OrderSettled event
  • Renames a variable - A variable named asyncOrderId actually represents the accountId
  • Updates all docs using docgen

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #1696 (d42307a) into main (9fa0cde) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1696   +/-   ##
=======================================
  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

@0xjocke
Copy link
Contributor

0xjocke commented Jun 30, 2023

Sorry I missed this draft. We might get some conflicts #1697

@Tburm
Copy link
Contributor Author

Tburm commented Jun 30, 2023

@bachstatter All good, I'm going to leave a comment on your PR actually. I was getting new "stack too deep" errors so I think new values need to go in the runtime.

@0xjocke
Copy link
Contributor

0xjocke commented Jul 3, 2023

@bachstatter All good, I'm going to leave a comment on your PR actually. I was getting new "stack too deep" errors so I think new values need to go in the runtime.

Yea you can see how I worked around it by moving the trackingCode to runtime. Solidity wont allow you to move PerpsMarket to runtime since runtime is memory and the market will be storage.. Luckily we could move the trackingCode and "afford" to have market as a variable

@0xjocke
Copy link
Contributor

0xjocke commented Jul 5, 2023

We probably want some of these changes.. I added size delta to the new MarketUpdated event. Do you still want a size delta on order settled?

@Tburm
Copy link
Contributor Author

Tburm commented Jul 5, 2023

@bachstatter Yes, we definitely want sizeDelta on OrderSettled. It will be easiest to calculate volume if we can just do sizeDelta * price from OrderSettled.

@Tburm Tburm closed this Jul 5, 2023
@noisekit noisekit deleted the order-events branch November 13, 2023 08:33
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