Skip to content
This repository was archived by the owner on Feb 2, 2024. It is now read-only.

feat: update internal buffer order graph #504

Merged
merged 10 commits into from
May 30, 2023

Conversation

alfetopito
Copy link
Collaborator

@alfetopito alfetopito commented May 29, 2023

Update

Using CoW Protocol Buffer as the node name

Moved the node together with the receiver address to be explicit this is a trade

Screenshot 2023-05-29 at 17 40 35

Summary

Part of #491
Follow up to #492

Added new node for Internal buffer trades

image

Tooltip includes only from and amount

image

To Test

  1. Open the app and search for the settlement hash 0xd78b614d0d5c39d55c516653e8674b73133d2c9bbdf302e5be036de7c6b304e6
  2. Go to graph tab
  • It should show a new node named Internal buffer
  1. Open a regular settlement hash such as 0xa6e62c2713ef08d4e979d8ba63643d6d1f522f8b825c27682098578dd9e1f043
  • It should be displayed as usual, without internal buffers node

@alfetopito alfetopito self-assigned this May 29, 2023
@vercel
Copy link

vercel bot commented May 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback

📖 Storybook ↗︎

@alfetopito alfetopito requested review from a team May 29, 2023 14:58
value: log.inputs[IndexTransferInput.value].value,
isInternal: from === to,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Assuming only internal trades will ever have same from & to

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, im not even sure is the case

It would be weird that there's an order under this case, it means we spend gas just to do nothing. And we make the user pay some gas. So doesn't look like a legit case, and I don't think the protocol should support wash-trading

Copy link
Contributor

Choose a reason for hiding this comment

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

so im fine with this assumption

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved the node somewhere else, to be clearer there's a trade involved.

Still, if not isInternal, what's your suggestion?

Comment on lines +136 to +146
const transferAddresses = new Set()

transfers.forEach((transfer) => {
transferAddresses.add(transfer.from)
transferAddresses.add(transfer.to)
})

try {
contracts
.filter((contract: Contract) => {
// Only usecontracts which are involved in a transfer
return transfers.find((transfer) => {
return transfer.from === contract.address || transfer.to === contract.address
})
})
.forEach((contract: Contract) => {
contracts.forEach((contract: Contract) => {
// Only use contracts which are involved in a transfer
if (transferAddresses.has(contract.address))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Optimization as it was searching the transfers list for every contract

@@ -121,6 +121,7 @@ export function STYLESHEET(theme: DefaultTheme): Stylesheet[] {
'curve-style': 'bezier',
'font-size': '15px',
'text-background-padding': '3px',
'control-point-step-size': 75,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Giving more space between edges

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Not for this PR, and maybe not for now, but maybe we will want to:

  • Label some special accounts, like: 0xa03be496e67ec29bc62f01a428683d7f9c204930
  • Also, provide a way to open a node in etherscan or something. it's hard to check the actually full address

Nothing you should address here, just some thoughts

value: log.inputs[IndexTransferInput.value].value,
isInternal: from === to,
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, im not even sure is the case

It would be weird that there's an order under this case, it means we spend gas just to do nothing. And we make the user pay some gas. So doesn't look like a legit case, and I don't think the protocol should support wash-trading

value: log.inputs[IndexTransferInput.value].value,
isInternal: from === to,
Copy link
Contributor

Choose a reason for hiding this comment

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

so im fine with this assumption

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

It works great!

@alfetopito alfetopito merged commit 92f0c31 into develop May 30, 2023
@alfetopito alfetopito deleted the 491/settlement-contract-trader-graph branch May 30, 2023 14:44
@alfetopito alfetopito mentioned this pull request May 31, 2023
@tukantje tukantje mentioned this pull request Jun 1, 2023
@elena-zh
Copy link

LGTM!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants