Sip 58 emit exchange entry settlements #553
Conversation
|
|
||
| // ========== EVENTS ========== | ||
|
|
||
| event ExchangeEntryAppended( |
There was a problem hiding this comment.
these events all need the timestamp of the original exchange, it was in the spec: https://sips.synthetix.io/sips/sip-58
This is what the dapps will use to try to connect the reclaim/rebates to the exchanges themselves cc @clementbalestrat
There was a problem hiding this comment.
@justinjmoses I realised that for events the better pattern is to use the block timestamp the event was emitted in (dapps can check the block.timestamp) instead of adding an extra timestamp to the data.
Codecov Report
@@ Coverage Diff @@
## develop #553 +/- ##
========================================
Coverage 99.44% 99.44%
========================================
Files 44 44
Lines 2712 2726 +14
Branches 378 378
========================================
+ Hits 2697 2711 +14
Misses 15 15
Continue to review full report at Codecov.
|
| uint256 amountReceived, | ||
| uint256 exchangeFeeRate, | ||
| uint256 roundIdForSrc, | ||
| uint256 roundIdForDest |
There was a problem hiding this comment.
You don't think it's worth just emitting the block.timestamp as well to aid in event correlation?
There was a problem hiding this comment.
I think it's easy for the dapps / synthetix exchange to correlate ExchangeEntryAppended event using the block + timestamp it was included in. @clementbalestrat any thoughts on this?
There was a problem hiding this comment.
Isn't it supposed to be the same?
There was a problem hiding this comment.
The question is do we include timestamp in the ExchangeEntryAppended event to match the one in ExchangeEntrySettled or does the dApp simply lookup the timestamp via an evm call to block.timestamp for ExchangeEntryAppended. I think more the former to be explicit, but we want your input @clementbalestrat
…8-emit-exchange-entry-settlements
…8-emit-exchange-entry-settlements
…8-emit-exchange-entry-settlements
… of ExchangeEntrySettlement
| } | ||
|
|
||
| mapping(address => mapping(bytes32 => ExchangeEntry[])) public exchanges; | ||
| mapping(address => mapping(bytes32 => IExchangeState.ExchangeEntry[])) public exchanges; |
There was a problem hiding this comment.
@justinjmoses If ExchangeState isn't redeployed it should be fine. The updated IExchangeState interface will be compiled when the new Exchanger contract is deployed.
…8-emit-exchange-entry-settlements
|
Updated. Please take a look |
|
Gas reports look pretty good! Just a heads up to not merge this into develop yet - let's wait until after the release on Tuesday in case we need to run a hotfix through |
| new web3.utils.BN(0), | ||
| new web3.utils.BN(1), | ||
| new web3.utils.BN(3), | ||
| exchangeTime + 1, |
There was a problem hiding this comment.
How do you know it's +1? Is it due to how buidler evm mines?
There was a problem hiding this comment.
Correct, buidler evm increments each txn / block by 1 second
No description provided.