refactor: events must emit addresses involved in the operation for better dApp indexing #186 #187
Conversation
There was a problem hiding this comment.
Finishing the review of the HotTask to provide adequate feedback!
You didn't win this round @blackbeard002 🏴☠️ so hoist the sails and meet you in the next round!
| @@ -36,7 +36,9 @@ contract Swaplace is SwapFactory, ISwaplace, IERC165 { | |||
|
|
|||
| _swaps[swapId] = swap; | |||
|
|
|||
| emit SwapCreated(swapId); | |||
| (address allowed,) = parseData(swap.config); | |||
There was a problem hiding this comment.
There is supposed to be a spacing after the comma like "(address allowed, )"
| emit SwapCreated(swapId); | ||
| (address allowed,) = parseData(swap.config); | ||
|
|
||
| emit SwapCreated(swapId, swap.owner, allowed); |
There was a problem hiding this comment.
We use msg.sender instead of the swap.owner, because the gas consumption will be lower.
| @@ -107,7 +109,7 @@ contract Swaplace is SwapFactory, ISwaplace, IERC165 { | |||
|
|
|||
| _swaps[swapId].config = 0; | |||
|
|
|||
| emit SwapCanceled(swapId); | |||
| emit SwapCanceled(swapId, _swaps[swapId].owner); | |||
There was a problem hiding this comment.
The msg.sender is the one that canceled the swap, so we should use msg.sender instead of reading from memory the swap owner.
There was a problem hiding this comment.
The prettierrc was not used in this file, making long lines instead of breaking them at 80 width
There was a problem hiding this comment.
The acceptee variable was not renamed into allowed as specified in the issue
Closes #186