feat: Add protection against computing LP fees for future quote timestamps #944
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes ACX-1537 Removes a lot of code needed to support deprecated config store variable that is always set to 0 in production
* fix(Dataworker): Dereference originChainId by index, not key Temporary fix to resolve an issue with future quoteTimestamps. We need a more robust solution than this in the long run. * Extract originToken from array * lint * WIP Signed-off-by: Matt Rice <[email protected]> --------- Signed-off-by: Matt Rice <[email protected]> Co-authored-by: Matt Rice <[email protected]>
* improve: leverage default TTL * improve: set fn signature to default
* feat: add dataworker force bundle range * docs: add documentation to example env * improve: allow for boba * improve: allow forced proposal range with sending --------- Co-authored-by: Paul <[email protected]>
* feat: Script for automating SpokePool deposits This is useful for making deposits to the zkSync SpokePool(s) because zkSync's block explorer doesn't yet support making transactions via proxy contracts. * Add yarn script target + tweak usage * Add basic support for dumping SpokePool config * lint * fix: Gracefully handle token searches where token missing Not all tokens are defined on all chains, so don't try to drop case unless the token actually exists on a chain. * Add "fetch" for dumping deposit and fill information Currently limitations: - Does not display updated values (i.e. after a speed-up). - Does not display message data. - Should normalise from the token decimals for better readability. - Does not gracefully handle when the txnHash is not found. * lint * fix conflict * lint * chore: allow base-units to easily be sent * chore: allow token to be passed as origin token addr or symbol * feat: test chain routes * nit: improve docs * docs: add docs * improve: account for checksummed value Co-authored-by: Paul <[email protected]> * improve: remove unneeded flag Co-authored-by: Paul <[email protected]> * improve: docs Co-authored-by: Paul <[email protected]> * improve: do not hardcode amounts * improve: modification * improve: dynamically compute price --------- Co-authored-by: Paul <[email protected]> Co-authored-by: nicholaspai <[email protected]>
This reverts commit 381e5bf.
…tamps This resolves the [reverted SDK PR](across-protocol/sdk#399) and re-introduces protection from deposit quote timestamps that are impossible to compute. This PR essentially moves that protection to the production version of Hub and Spoke clients so that the unit tests can continue to work and not have to add additional logic to set quote times correctly. Ideally in the future we set the quote times how they would in production but for now this solution results in a much smaller code diff
nicholaspai
changed the title
nicholaspai
requested review from
mrice32,
pxrl and
james-a-morris
as code owners
* chore: bump sdk to latest * chore: bump version * chore: bump sdk
nicholaspai
force-pushed
the
npai/transfer-threshold-removal
branch
from
d7b9666 to
90cb0d1
Compare
Comment on lines
+23
to
+37
| async computeRealizedLpFeePct( | ||
| deposit: Pick< | ||
| DepositWithBlock, | ||
| "quoteTimestamp" | "amount" | "destinationChainId" | "originChainId" | "blockNumber" | ||
| >, | ||
| l1Token: string | ||
| ): Promise<{ realizedLpFeePct: BigNumber | undefined; quoteBlock: number }> { | ||
| if (deposit.quoteTimestamp > this.currentTime) { | ||
| throw new Error( | ||
| `Cannot compute lp fee percent for quote timestamp ${deposit.quoteTimestamp} in the future. Current time: ${this.currentTime}.` | ||
| ); | ||
| } | ||
|
|
||
| return await super.computeRealizedLpFeePct(deposit, l1Token); | ||
| } |
There was a problem hiding this comment.
Could we add this check directly into the SDK?
There was a problem hiding this comment.
Well, we could, but it seriously messes up the tests :) The main point of putting it here is we don't actually test this client. A lot of the tests make don't opine on the quote timestamps so we'd have to change a lot
james-a-morris
approved these changes
Sep 28, 2023
nicholaspai
added a commit
that referenced
this pull request
Sep 29, 2023
* improve(dataworker): Remove `transferThreshold` Fixes ACX-1537 Removes a lot of code needed to support deprecated config store variable that is always set to 0 in production * Update Dataworker.ts * Update Dataworker.ts * Update package.json * Update yarn.lock * wip * WIP * Update yarn.lock * Revert "WIP" This reverts commit 381e5bf. * chore: bump sdk to latest (#943) * chore: bump sdk to latest * chore: bump version * chore: bump sdk * Update src/dataworker/Dataworker.ts Co-authored-by: Paul <[email protected]> * feat: Add protection against computing LP fees for future quote timestamps (#944) * improve(dataworker): Remove `transferThreshold` Fixes ACX-1537 Removes a lot of code needed to support deprecated config store variable that is always set to 0 in production * Update Dataworker.ts * bumpity * Update Dataworker.ts * Update package.json * Update yarn.lock * wip * fix(Dataworker): Dereference originChainId by index, not key (#936) * fix(Dataworker): Dereference originChainId by index, not key Temporary fix to resolve an issue with future quoteTimestamps. We need a more robust solution than this in the long run. * Extract originToken from array * lint * WIP Signed-off-by: Matt Rice <[email protected]> --------- Signed-off-by: Matt Rice <[email protected]> Co-authored-by: Matt Rice <[email protected]> * improve: leverage default TTL & leverage caching (#938) * improve: leverage default TTL * improve: set fn signature to default * feat: add dataworker force bundle range (#912) * feat: add dataworker force bundle range * docs: add documentation to example env * improve: allow for boba * improve: allow forced proposal range with sending --------- Co-authored-by: Paul <[email protected]> * feat: test chain routes (#897) * feat: Script for automating SpokePool deposits This is useful for making deposits to the zkSync SpokePool(s) because zkSync's block explorer doesn't yet support making transactions via proxy contracts. * Add yarn script target + tweak usage * Add basic support for dumping SpokePool config * lint * fix: Gracefully handle token searches where token missing Not all tokens are defined on all chains, so don't try to drop case unless the token actually exists on a chain. * Add "fetch" for dumping deposit and fill information Currently limitations: - Does not display updated values (i.e. after a speed-up). - Does not display message data. - Should normalise from the token decimals for better readability. - Does not gracefully handle when the txnHash is not found. * lint * fix conflict * lint * chore: allow base-units to easily be sent * chore: allow token to be passed as origin token addr or symbol * feat: test chain routes * nit: improve docs * docs: add docs * improve: account for checksummed value Co-authored-by: Paul <[email protected]> * improve: remove unneeded flag Co-authored-by: Paul <[email protected]> * improve: docs Co-authored-by: Paul <[email protected]> * improve: do not hardcode amounts * improve: modification * improve: dynamically compute price --------- Co-authored-by: Paul <[email protected]> Co-authored-by: nicholaspai <[email protected]> * WIP * Update yarn.lock * Revert "WIP" This reverts commit 381e5bf. * feat: Add protection against computing LP fees for future quote timestamps This resolves the [reverted SDK PR](across-protocol/sdk#399) and re-introduces protection from deposit quote timestamps that are impossible to compute. This PR essentially moves that protection to the production version of Hub and Spoke clients so that the unit tests can continue to work and not have to add additional logic to set quote times correctly. Ideally in the future we set the quote times how they would in production but for now this solution results in a much smaller code diff * lint * fix * lint * Update HubPoolClient.ts * refactor: Import getNetworkName from sdk (#945) * chore: bump sdk to latest (#943) * chore: bump sdk to latest * chore: bump version * chore: bump sdk --------- Signed-off-by: Matt Rice <[email protected]> Co-authored-by: Paul <[email protected]> Co-authored-by: Matt Rice <[email protected]> Co-authored-by: James Morris, MS <[email protected]> --------- Signed-off-by: Matt Rice <[email protected]> Co-authored-by: James Morris, MS <[email protected]> Co-authored-by: Paul <[email protected]> Co-authored-by: Matt Rice <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This resolves the reverted SDK PR and re-introduces protection from deposit quote timestamps that are impossible to compute.
This PR essentially moves that protection to the production version of Hub and Spoke clients so that the unit tests can continue to work and not have to add additional logic to set quote times correctly. Ideally in the future we set the quote times how they would in production but for now this solution results in a much smaller code diff