-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: assign spoke pool events to bundles #77
feat: assign spoke pool events to bundles #77
Conversation
|
||
@Entity() | ||
@Unique("UK_bundleEvents_eventType_relayHash", ["eventType", "relayHash"]) | ||
export class BundleEvents { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we use to name entities as singular noun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed here 09c466a
} from "typeorm"; | ||
import { Bundle } from "./Bundle"; | ||
|
||
export enum BundleEventTypes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: singular noun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed here too 09c466a
bundleId: number; | ||
|
||
@Column({ type: "enum", enum: BundleEventTypes }) | ||
eventType: BundleEventTypes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit, can we rename this to simply type
since this entity already is referring to an event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was also renamed here 09c466a
import { | ||
BundleDepositsV3, | ||
BundleExcessSlowFills, | ||
BundleFillsV3, | ||
BundleSlowFills, | ||
ExpiredDepositsToRefundV3, | ||
LoadDataReturnValue, | ||
} from "@across-protocol/sdk/dist/cjs/interfaces/BundleData"; | ||
import { getRelayHashFromEvent } from "@across-protocol/sdk/dist/cjs/utils/SpokeUtils"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be imported differently without using the dist
dir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here 1747f18
bundleId: number, | ||
): { | ||
bundleId: number; | ||
relayHash: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the relayHash
a string? Is there an edge case because of which we need any
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops! Fixed here 09c466a
bundleData.bundleDepositsV3, | ||
bundleId, | ||
); | ||
const storedDeposits = await eventsRepo.insert(formattedDeposits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these inserts should be split in multiple chunks with fixed length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! Done here 670701c
@@ -1,7 +1,8 @@ | |||
import { utils, clients } from "@across-protocol/sdk"; | |||
import { LoadDataReturnValue } from "@across-protocol/sdk/dist/cjs/interfaces/BundleData"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the type be imported only from the /dist/cjs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here 1747f18
[CHAIN_IDs.REDSTONE]: 2, | ||
[CHAIN_IDs.SCROLL]: 3, | ||
[CHAIN_IDs.ZK_SYNC]: 1, | ||
[CHAIN_IDs.ZORA]: 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add world chain too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!blockTime) { | ||
return 10; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return a value or throw an error to surface the missing value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for now is ok to return a value as we are using this just to reconstruct the bundles.
4a33616
to
373a698
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cant say my approval here has much weight
fromBlock: utils.ACROSS_V3_MAINNET_DEPLOYMENT_BLOCK, | ||
}); | ||
|
||
if (executedBundles.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just return early rather than nesting the rest of the logic
This PR adds the functionality to reconstruct executed bundles in order to identify the events that were part of it and save them in the database. It also uses the stored events to identify refunds of expired deposits.