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

Adding Competition Solver tab to settlements page #590

Closed

Conversation

0xaaiden
Copy link

Summary

This pull request introduces a new feature to the settlements page - the "Competition Solver tab". This tab provides a detailed view of solver competitions, showing relevant data such as Auction ID, Auction Start Block, and more. The intention behind this tab is to give users a comprehensive overview of solver competitions, aiding in transparency and insight.

image

To Test

  1. Access the Competition Solver tab:
  • Open the settlements page ../tx/{txid} .
  • Navigate to the new "Competition Solver tab".
  • Expectation: The tab should display data entries like Auction ID, Auction Start Block, Liquidity Collected Block, etc.
  • The data should be organized in rows with copy buttons, aiding easy sharing.
  1. Interact with UI elements:
  • Try the copy button to check if data gets copied to clipboard efficiently.
  • Hover over the tooltips to see concise explanations.
  • Tooltips should provide a clear brief about each entry.

@vercel
Copy link

vercel bot commented Aug 18, 2023

Someone is attempting to deploy a commit to the cow Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Aug 22, 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 Aug 28, 2023 9:46am

Comment on lines +168 to +178
export async function getSolverCompetitionByTx(params: GetTxSolverCompetitionParams): Promise<RawSolverCompetition> {
const { networkId, txHash } = params

console.log(
`[getSolverCompetitionByTx] Fetching Solver Competition on network ${networkId} with filters: tx_hash=${txHash}`,
)

const queryString = '/solver_competition/by_tx_hash/' + txHash

return _fetchQuery(networkId, queryString)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This unfortunately doesn't work with prod batches in this PR, as it only queries the env corresponding to the page's env.

See for example this goerli batch

It returns a 404 for barn while it has the proper response on prod

But I think you'd be aware of that as it for sure happened while developing/testing.

Long story short, there are 2 databases, and we usually query the one corresponding to the loaded environment.
In some cases though, we need to query both.
Check for reference the implementation of getTxOrders:

https://github.com/cowprotocol/explorer/blob/develop/src/api/operator/operatorApi.ts#L104-L119

Also, this endpoint should be moved to the https://github.com/cowprotocol/cow-sdk/. I'll raise this internally and see that we include it in the next release. No action from your part on this regard, just mentioning it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Upon further investigation, it seems that even batches which were made against barn still don't return anything in the solver endpoint for the same env, such as the one I intentionally created https://explorer-dev-git-fork-0xaaiden-feature-transacti-ee2e16-cowswap.vercel.app/gc/tx/0x540537ad6d2d7948a232f39ec7e91c66f2c0a476d5ca84617a2fec2d3eb5ce54?tab=solver

Looks like it's something on the backend side.

A quick fix would be to always query prod, although I don't think querying both requires much effort.

I'll reach out internally about this as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got an answer that clarifies why it suddenly became available.

Searching by hash takes 65 blocks to propagate (we wait for the block to be final). If you know the auction id you can search immediately using that identifier

I suggest you to watch for the current block and compare with the tx block.
While the different is < 65, don't bother querying the backend, and show some indication in the UI saying the data is not yet available.

@alfetopito
Copy link
Collaborator

One more feedback

On gchain, the native token name is xDai
image

There's a mapping for that https://github.com/cowprotocol/explorer/blob/develop/src/const.ts#L183

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's not needed, the blocknumber can be fetched from the chain using the web3 api

const web3 = createWeb3Api()

Just call web3.eth.getBlockNumber() https://docs.web3js.org/api/web3-eth/function/getBlockNumber/

You could add the blocknumber to the global state and have an updater running every ~15s.
Or have a more "local" state and instantiate the updater only when needed, which is when the tx details page is loaded & the block difference is < 65.

I'm going too far with ideas, I'll let you take your pick or propose something else.

Copy link
Author

Choose a reason for hiding this comment

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

Changes made as advertised, just trying to keep it simple and clear

Comment on lines 28 to 29
OPERATOR_URL_STAGING_MAINNET: 'https://barn.api.cow.fi/mainnet/api',
OPERATOR_URL_STAGING_GOERLI: 'https://barn.api.cow.fi/goerli/api',
OPERATOR_URL_STAGING_XDAI: 'https://barn.api.cow.fi/xdai/api',
OPERATOR_URL_STAGING_MAINNET: 'https://api.cow.fi/mainnet/api',
OPERATOR_URL_STAGING_GOERLI: 'https://api.cow.fi/goerli/api',
OPERATOR_URL_STAGING_XDAI: 'https://api.cow.fi/xdai/api',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't change that.

What you need is in this comment which still applies https://github.com/cowprotocol/explorer/pull/590/files#r1301457728

Copy link

Choose a reason for hiding this comment

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

@alfetopito adressed

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.

As a meta-feedback, the PR introduces 1.2K new lines of code. This makes it hard for us to thoroughly review such a big PR.

Ideally, it should be a more iterative process.

Also, this PR define lots of types that should be part of the SDK

@@ -1,5 +1,5 @@
import React, { useCallback, useEffect, useState } from 'react'
import { faListUl, faProjectDiagram } from '@fortawesome/free-solid-svg-icons'
import { faListUl, faProjectDiagram, faHandshakeAngle } from '@fortawesome/free-solid-svg-icons'
Copy link
Contributor

Choose a reason for hiding this comment

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

@fairlighteth any suggestion for the tab icon?

}
const BoringAvatar: React.FC<BoringAvatarProps> = ({ alt }) => {
const [colors] = useState<string[]>(
Array.from({ length: 6 }, () => Math.floor(Math.random() * 16777215).toString(16)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this random logic?

also, nit, since you are creating a component call BoringAvatar, should you call the file with the same name?

Copy link

Choose a reason for hiding this comment

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

Was getting a lot of close colors avatars without it.

)
return <Avatar alt={alt} src={`https://source.boringavatars.com/pixel/120/${alt}?colors=${colors.join(',')}`} />
}
export default BoringAvatar
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would avoid default export. With named one is enough

Copy link

Choose a reason for hiding this comment

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

Ok

[o.sellTokenAddress]: o.sellToken,
})),
)
console.log(tokens)
Copy link
Contributor

Choose a reason for hiding this comment

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

delete console logs

const ClearingPrices: React.FC<Props> = (props) => {
const { orders, prices } = props
const networkId = useNetworkId() ?? undefined
const tokens: { [p: string]: TokenErc20 | null | undefined } = Object.assign(
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be meoized?

const { ranking, solver, solverAddress } = solution ?? {}
const [open, setOpen] = useState<boolean>(false)
const isMobile = useMediaBreakpoint(['xs', 'sm'])
const network = useNetworkId() || 1
Copy link
Contributor

Choose a reason for hiding this comment

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

no magic numbers. We probably have some defaults already

Fees <HelpTooltip tooltip={tooltip.surplus} />
</HeaderTitle>
<HeaderValue>
{formatNumbers(fees)} {NATIVE_TOKEN_PER_NETWORK[network].symbol}
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a lot of repetition. Just create consts

<td>
<IconButton className={'mediumUp'} aria-label="expand row" size="small" onClick={(): void => setOpen(!open)}>
{open ? (
<FontAwesomeIcon color={'#ffffff'} icon={faChevronUp} />
Copy link
Contributor

Choose a reason for hiding this comment

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

hardcoded colors

<RowWithCopyButton
textToCopy={data.auctionStartBlock.toString()}
contentsToDisplay={data.auctionStartBlock.toString()}
onCopy={(): void => onCopy('auctionId')}
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid repetition

`[getSolverCompetitionByTx] Fetching Solver Competition on network ${networkId} with filters: tx_hash=${txHash}`,
)

const queryString = '/solver_competition/by_tx_hash/' + txHash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just noticed, this endpoint is available from orderBookSDK since version https://github.com/cowprotocol/cow-sdk/releases/tag/v2.3.0

Update to the latest and you can use it directly.

You will still HAVE TO query both barn and prod envs, like it's done here: https://github.com/cowprotocol/explorer/blob/develop/src/api/operator/operatorApi.ts#L104-L119

@vicebas
Copy link

vicebas commented Aug 29, 2023

As a meta-feedback, the PR introduces 1.2K new lines of code. This makes it hard for us to thoroughly review such a big PR.

Ideally, it should be a more iterative process.

Also, this PR define lots of types that should be part of the SDK

The types on the SDK are missing the auctionStartBlock, for some reason


export async function getSolverCompetitionByTx(params: GetTxSolverCompetitionParams): Promise<RawSolverCompetition[]> {
const { txHash } = params
const solverCompetition = { ...RAW_SOLVER_COMPETITION }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick:

return [{
    ...RAW_SOLVER_COMPETITION,
    transactionHash: txHash || solverCompetition.transactionHash
}]

const { token, network, amount } = props

const [invertedPrice, setInvertedPrice] = useState<boolean>(false)
console.log(token, amount.toNumber())
Copy link
Contributor

Choose a reason for hiding this comment

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

console.log should be removed


const [invertedPrice, setInvertedPrice] = useState<boolean>(false)
console.log(token, amount.toNumber())
const calculatedPrice = amount.div(TEN_BIG_NUMBER.exponentiatedBy(36 - token.decimals.valueOf()))
Copy link
Contributor

Choose a reason for hiding this comment

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

What is 36? Would be great to move it to a const with an appropriate name + comment

</PricesCard>
)
}
export default ClearingPrices
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use named exports instead of default

}
getCurrentBlock()
}, [web3])
if (isLoadingTransactionData || !currentBlock) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ubernit:
Please, add a blank line between useEffect() and if() to improve the code readability

@@ -32,9 +38,15 @@ async function _fetchErc20FromNetwork(params: {
// Causing the caller to never know we got the token it was looking for
return { ...nativeToken, address }
}

const cacheKey = `${networkId}-${address}`
const cachedData = getCachedData(cacheKey, ERC20_INFO_CACHE)
Copy link
Contributor

Choose a reason for hiding this comment

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

getCachedData looks redundant. Bellow we just use ERC20_INFO_CACHE.set() I think here we also can just ERC20_INFO_CACHE.get()

@@ -12,7 +12,7 @@ function checkEnvironment(host: string): EnvsFlags {
const domainStagingRegex = getRegex(process.env.EXPLORER_APP_DOMAIN_REGEX_STAGING)
const domainProdRegex = getRegex(process.env.EXPLORER_APP_DOMAIN_REGEX_PROD)
const domainBarnRegex = getRegex(process.env.EXPLORER_APP_DOMAIN_REGEX_BARN)

console.log('host', domainStagingRegex, domainStagingRegex?.test(host), host)
Copy link
Contributor

Choose a reason for hiding this comment

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

console.log

setSolverCompetition(cachedData)
} else {
const solverCompetition = await getSolverCompetitionByTx({ networkId: network, txHash: _txHash })
console.log('solverCompetition', solverCompetition)
Copy link
Contributor

Choose a reason for hiding this comment

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

console.log

Copy link
Contributor

@shoom3301 shoom3301 left a comment

Choose a reason for hiding this comment

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

This is amazing and huge work! Thank you!
The changes need some polishing.

@alfetopito
Copy link
Collaborator

As a meta-feedback, the PR introduces 1.2K new lines of code. This makes it hard for us to thoroughly review such a big PR.
Ideally, it should be a more iterative process.
Also, this PR define lots of types that should be part of the SDK

The types on the SDK are missing the auctionStartBlock, for some reason

That is true. I checked and the SDK version is outdated. Check this comment #590 (comment)

@shoom3301
Copy link
Contributor

Hi!
We don't see any activity in the PR for a long time.
Unfortunately, we have to close the PR since we are moving to another repo: cowprotocol/cowswap#3579.
We will be happy if you continue the work here.
Thank you!

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

Successfully merging this pull request may close these issues.

5 participants