Bid Status Visibility #42

anihamde · 2024-04-04T19:19:29Z

enable historical bid queries by querying directly from database
add an event to ExpressRelay contract to make included bids more visible on-chain
add auction table
create query for auction based on permission key
add tx hash to Lost status

m30m · 2024-04-10T08:31:59Z

auction-server/migrations/20240404172845_auction.up.sql

+CREATE TABLE auction
+(
+    id              UUID PRIMARY KEY,
+    conclusion_time TIMESTAMP      NOT NULL,


let's add another column creation_time with default current_timestamp to keep track of when we have added this row to the db. This can be helpful next to this conclusion_time to measure latency, etc.

m30m · 2024-04-10T08:46:10Z

auction-server/src/api/auction.rs

+    pub params: AuctionParams,
+}
+
+/// Query for auctions with the permission key and (optionally) chain ID specified.


I am not sure who is gonna use this endpoint, let's discuss the access patterns and how we want searchers to have more visibility.

m30m · 2024-04-10T08:51:57Z

auction-server/src/api/auction.rs

+    Ok(Json(auctions))
+}
+
+// Get auction with the specified ID.


no additional information in this comment.

m30m · 2024-04-10T08:52:51Z

auction-server/src/api/auction.rs

+    let auction = sqlx::query!("SELECT * FROM auction WHERE id = $1", auction_id)
+        .fetch_one(&store.db)
+        .await
+        .map_err(|_| RestError::BidNotFound)?;


should be AuctionNotFound

m30m · 2024-04-10T09:04:13Z

auction-server/src/api/auction.rs

+        params: AuctionParams {
+            chain_id:       auction.chain_id,
+            permission_key: auction.permission_key.into(),
+            tx_hash:        H256::from_slice(auction.tx_hash.as_ref()),


from_slice can panic. Isn't there any safer way to do this?

m30m · 2024-04-10T09:54:54Z

auction-server/src/state.rs

    ) -> anyhow::Result<()> {
-        if update.bid_status == BidStatus::Pending {


I think we can keep this as an additional safety measure.

m30m · 2024-04-10T10:03:30Z

per_multicall/src/ExpressRelayEvents.sol

+        bytes indexed permissionKey,
+        uint256 indexed multicallIndex,
+        uint256 bidAmount,
+        bool externalSuccess,


can't we just use the MulticallStatus type here?

m30m · 2024-04-10T10:11:18Z

per_multicall/src/ExpressRelayEvents.sol

+pragma solidity ^0.8.13;
+
+contract ExpressRelayEvents {
+    event ReceivedETH(address indexed sender, uint256 amount);


indexed adds some additional gas IIRC. Let's make sure this is worth it.

m30m · 2024-04-10T10:19:43Z

per_multicall/src/ExpressRelayEvents.sol

+    event ReceivedETH(address indexed sender, uint256 amount);
+    event MulticallIssued(
+        bytes indexed permissionKey,
+        uint256 indexed multicallIndex,


Can we replace this with bid-id? I think it will become much easier for searchers to listen to an event with the specific bid id we returned to them at the time of bid submission. Otherwise they have to wait until auction conclusion to get this multicallIndex. Otherwise, they have to use (tx_hash, permission_key,multicall_index) as the unique key for identifying their bid.

m30m · 2024-04-10T10:23:05Z

auction-server/migrations/20240326063340_bids.up.sql

@@ -10,6 +10,5 @@ CREATE TABLE bid
    target_calldata BYTEA          NOT NULL,
    bid_amount      NUMERIC(78, 0) NOT NULL,
    status          bid_status     NOT NULL,
-    auction_id      UUID,     -- TODO: should be linked to the auction table in the future
-    removal_time    TIMESTAMP -- TODO: should be removed and read from the auction table in the future
+    auction_id      UUID


let's add another column here that shows the position of this bid in the bundle. Having a null value while having an auction id means that this bid was not submitted on-chain (it lost)

We should add a foreign key with protect constraint.

m30m · 2024-04-11T12:41:32Z

auction-server/src/api/opportunity.rs

@@ -51,7 +51,7 @@ pub struct OpportunityParamsWithMetadata {
    opportunity_id: OpportunityId,
    /// Creation time of the opportunity


let's add a comment that this is in nanoseconds, also change the example to a more realistic value

m30m · 2024-04-11T12:44:55Z

auction-server/src/state.rs

+    pub async fn update_auction(&self, auction: AuctionParamsWithMetadata) -> anyhow::Result<()> {
+        let conclusion_datetime =
+            OffsetDateTime::from_unix_timestamp_nanos(auction.conclusion_time)?;
+        sqlx::query!("UPDATE auction SET conclusion_time = $1, tx_hash = $2 WHERE id = $3 AND permission_key = $4 AND chain_id = $5 AND conclusion_time IS NULL",


I don't get why we need AND permission_key = $4 AND chain_id = $5. Auction id is unique alone right?

m30m · 2024-04-11T12:50:38Z

auction-server/src/api/opportunity.rs

@@ -51,7 +51,7 @@ pub struct OpportunityParamsWithMetadata {
    opportunity_id: OpportunityId,
    /// Creation time of the opportunity
    #[schema(example = 1700000000, value_type = i64)]


m30m · 2024-04-11T12:53:21Z

auction-server/src/state.rs

+        #[schema(example = "0x103d4fbd777a36311b5161f2062490f761f25b67406badb2bace62bb170aa4e3", value_type = String)]
+        result: H256,
+        #[schema(example = 1, value_type = u64)]
+        index:  U256,


we can just use u64 or even u32 here

m30m · 2024-04-11T13:01:04Z

per_multicall/test/ExpressRelayTestSetup.sol

@@ -587,7 +587,13 @@ contract ExpressRelayTestSetup is

        multicallData = new MulticallData[](contracts.length);
        for (uint i = 0; i < contracts.length; i++) {
+            bytes16 bidId = bytes16(


this gives a false hint that this is how we construct the id. I'd rather use something simpler like bytes16(keccak256("randomstuff")) or even simpler than this if possible

this is to allow us to have distinct bid ids for future tests, that we can easily recover. i'll precede this construction with something like what you've suggested to make it clear this is not a valid construction

m30m · 2024-04-11T14:26:43Z

auction-server/src/state.rs

@@ -216,6 +230,7 @@ pub struct Store {
 }

 impl Store {


I think it's easy enough to find this out looking at the implementations. I'm not a big fan of comments TBH. Also, some comments are misleading here.
remove_opportunity and broadcast_bid_status_and_remove do not state that they access db

m30m · 2024-04-11T14:29:42Z

auction-server/src/api/bid.rs

-    match store.bids.read().await.get(&bid_id) {
-        Some(bid) => Ok(bid.status.clone().into()),
-        None => Err(RestError::BidNotFound),
+    let status_data = sqlx::query!(


I would move this to state. Also no need to have 2 separate queries, we can join these 2 tables and have a single query. I think the code will become simpler.

m30m · 2024-04-11T14:31:20Z

auction-server/src/api.rs

@@ -75,7 +83,11 @@ pub enum RestError {
    InvalidChainId,
    /// The simulation failed
    SimulationError { result: Bytes, reason: String },
-    /// The order was not found
+    /// The auction was not concluded
+    AuctionNotConcluded,


I think this should not be an error, Pending is a valid status

this is only called in get_concluded_auction, where we expect the auction being queried to have been concluded

m30m · 2024-04-11T16:44:34Z

auction-server/src/state.rs

+    Submitted {
+        #[schema(example = "0x103d4fbd777a36311b5161f2062490f761f25b67406badb2bace62bb170aa4e3", value_type = String)]
+        result: H256,
+        #[schema(example = 1, value_type = u32)]


no need to specify value_type if it's the same

m30m · 2024-04-11T17:40:39Z

auction-server/src/api/opportunity.rs

-    /// Creation time of the opportunity
-    #[schema(example = 1700000000, value_type = i64)]
-    creation_time:  UnixTimestamp,
+    /// Creation time of the opportunity (in nanoseconds since the Unix epoch)


update the comment

m30m · 2024-04-11T17:45:21Z

auction-server/src/api/opportunity.rs

-    #[schema(example = 1700000000, value_type = i64)]
-    creation_time:  UnixTimestamp,
+    /// Creation time of the opportunity (in nanoseconds since the Unix epoch)
+    #[schema(example = 1_700_000_000_000_000i128, value_type = i128)]


I think this still fits in i64. No need to introduce i128

--systemdf and others added 11 commits April 3, 2024 19:50

bid status querying from db

3ab5f4d

event for permission key

613c14d

add auction table

cbeeea6

more granularity on the events

6f7d698

add tx hash to Lost

5540b4b

add querying auctions via permission key

60f6285

add query param

8f433d6

adjust some formatting

51a561a

some cleanup

a7e9c76

tic

895b484

comments

5fb3f38

m30m reviewed Apr 10, 2024

View reviewed changes

--systemdf and others added 4 commits April 10, 2024 11:11

address comments 1

0c2499e

address comments 2

13c8ef1

convert ts to nanos

227eae8

some minor changes

329c517

m30m reviewed Apr 11, 2024

View reviewed changes

--systemdf added 3 commits April 11, 2024 11:06

updated cargo locks

e4d53e2

address comments

de3edd8

use stable cargo

e11b810

m30m approved these changes Apr 11, 2024

View reviewed changes

nanos -> micros & dumb nightly changes

9cc5efd

m30m reviewed Apr 11, 2024

View reviewed changes

--systemdf added 2 commits April 11, 2024 16:06

bump version

52abd3f

fix nano --> micro in schema doc

809d653

anihamde merged commit 3591750 into main Apr 11, 2024
1 check failed

anihamde deleted the bid_status_queries branch April 11, 2024 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bid Status Visibility #42

Bid Status Visibility #42

anihamde commented Apr 4, 2024 •

edited

Loading

m30m Apr 10, 2024

m30m Apr 10, 2024

m30m Apr 10, 2024

m30m Apr 10, 2024

m30m Apr 10, 2024

m30m Apr 10, 2024

m30m Apr 10, 2024

m30m Apr 10, 2024

m30m Apr 10, 2024

m30m Apr 10, 2024

m30m Apr 10, 2024

m30m Apr 11, 2024

m30m Apr 11, 2024

m30m Apr 11, 2024

m30m Apr 11, 2024

m30m Apr 11, 2024

anihamde Apr 11, 2024

m30m Apr 11, 2024

m30m Apr 11, 2024

m30m Apr 11, 2024

anihamde Apr 11, 2024

m30m Apr 11, 2024

m30m Apr 11, 2024

m30m Apr 11, 2024

		) -> anyhow::Result<()> {
		if update.bid_status == BidStatus::Pending {

		@@ -51,7 +51,7 @@ pub struct OpportunityParamsWithMetadata {
		opportunity_id: OpportunityId,
		/// Creation time of the opportunity

Bid Status Visibility #42

Bid Status Visibility #42

Conversation

anihamde commented Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anihamde commented Apr 4, 2024 •

edited

Loading