improve(dataworker): Remove `transferThreshold` #937

nicholaspai · 2023-09-13T19:17:58Z

Fixes ACX-1537

Removes a lot of code needed to support deprecated config store variable that is always set to 0 in production

linear · 2023-09-13T19:18:02Z

ACX-1537 Remove `transferThreshold` from ConfigStore

This config was deprecated and replaced by spokeTargetBalances and has a lot of code/unit tests that are no longer useful

…ble from config store Upstream of across-protocol/relayer#937

nicholaspai · 2023-09-21T19:10:04Z

Can't merge this until the VERSION is bumped to 2 in the ConfgiStore contract

nicholaspai · 2023-09-21T19:10:10Z

Can't merge this until the VERSION is bumped to 2 in the ConfgiStore contract

Fixes ACX-1537 Removes a lot of code needed to support deprecated config store variable that is always set to 0 in production

This reverts commit 381e5bf.

* chore: bump sdk to latest * chore: bump version * chore: bump sdk

nicholaspai · 2023-09-27T16:14:28Z

@james-a-morris @pxrl this is ready to be re-reviewed now and works locally against prod

james-a-morris · 2023-09-27T16:16:06Z

@james-a-morris @pxrl this is ready to be re-reviewed now and works locally against prod

SGTM - will add this to my plate

src/dataworker/Dataworker.ts

pxrl · 2023-09-28T15:57:05Z

test/SpokePoolClient.fills.ts

Just to confirm, is this necessitated by the realizedLpFeePct change?

I like that this simplifies the test quite a bit, but I also sometimes like that we verify events at a deeper level, just to be sure that we aren't tricking ourselves somehow.

So for some reason the object key order changes when I make changes to the interface of the Fill object. This is probably some weird nodejs thing. I also think we verify the deep properties of the object in other places

Is it confirmed related to changes in the Fill object, or could it be related to the bump in ts that happened as part of the tsdx migration? That happened in the bump to 0.16.1, so we are bundling that in here.

(As a background thought, I'm wondering if we might consider bumping to 0.16.1 first in order to rule out any potential regressions as part of that?)

Ive confirmed its the order of the keys switching

Also these changes i noticed before we integrated tsdx so i don't think its related

pxrl · 2023-09-28T15:59:00Z

test/TokenClient.TokenShortfall.ts


    // Updating the client should not impact anything.
    await updateAllClients();
-    expect(deepEqualsWithBigNumber(tokenClient.getTokenShortfall(), expectedData2)).to.be.true;
+    expect(tokenShortFallData2).to.deep.equal(tokenClient.getTokenShortfall()[destinationChainId][erc20_2.address]);


I vaguely recall that we previously had problems with to.deep.equal on BigNumbers (i.e. it could sometimes incorrectly report equality). Do you know if that can still pose an issue?

Hm I'm not sure exactly. I changed this because the deepEquals didn't seem to be deeply traversing the object as expected

Per #937 (comment), I'm wondering if there's some subtle change as a result of bumping tsc. We skipped a lot of versions.

Co-authored-by: Paul <[email protected]>

…tamps (#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]>

mrice32

LGTM!

mrice32 · 2023-09-28T23:42:19Z

src/clients/HubPoolClient.ts

@@ -18,4 +19,20 @@ export class HubPoolClient extends clients.HubPoolClient {
      ignoredHubProposedBundles: IGNORED_HUB_PROPOSED_BUNDLES,
    });
  }
+
+  async computeRealizedLpFeePct(


nit: I think this code from a different branch accidentally got included here. These commits should be removed (or just merge master) before merging.

nope, that other PR was merged into this branch!

mrice32 · 2023-09-28T23:43:45Z

package.json

@@ -11,7 +11,7 @@
  },
  "dependencies": {
    "@across-protocol/contracts-v2": "2.4.3",
-    "@across-protocol/sdk-v2": "0.15.24",
+    "@across-protocol/sdk-v2": "0.16.4",


How did we get this far behind...? Can you just double check the changes that get pulled in here (considering the number of bumps)?

I believe 0.15.24 was the last release before 0.16 and that's because https://github.com/across-protocol/sdk-v2/releases/tag/v0.16.0 introduced the removal of the transferThreshold variable!

This PR has been sitting for a while haha

mrice32

LGTM! Thanks for the fast response!

nicholaspai added a commit to across-protocol/sdk that referenced this pull request Sep 13, 2023

improve(ConfigStoreClient): Remove deprecated transferThreshold varia…

25411f0

…ble from config store Upstream of across-protocol/relayer#937

nicholaspai mentioned this pull request Sep 13, 2023

improve(ConfigStoreClient): Remove deprecated transferThreshold variable from config store across-protocol/sdk#388

Merged

nicholaspai added a commit to across-protocol/sdk that referenced this pull request Sep 19, 2023

improve(ConfigStoreClient): Remove deprecated transferThreshold varia…

31794ec

…ble from config store Upstream of across-protocol/relayer#937

nicholaspai mentioned this pull request Sep 20, 2023

chore: Update SDK-v2 version so that UBA min version is bumped #941

Merged

nicholaspai marked this pull request as ready for review September 21, 2023 19:08

nicholaspai requested review from mrice32, pxrl and james-a-morris as code owners September 21, 2023 19:08

nicholaspai added the do not merge Don't merge until label is removed label Sep 21, 2023

nicholaspai removed the do not merge Don't merge until label is removed label Sep 26, 2023

nicholaspai and others added 10 commits September 26, 2023 10:18

improve(dataworker): Remove transferThreshold

158920d

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

6d5e600

Update Dataworker.ts

f12e3c7

Update package.json

af0b889

Update yarn.lock

8a11383

wip

0c769bb

WIP

819a96d

Update yarn.lock

2871b09

Revert "WIP"

c4c9a71

This reverts commit 381e5bf.

chore: bump sdk to latest (#943)

90cb0d1

* 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 September 26, 2023 14:18

nicholaspai added 2 commits September 26, 2023 10:24

Merge branch 'master' into npai/transfer-threshold-removal

2a488fe

Merge branch 'master' into npai/transfer-threshold-removal

d56c7c2

pxrl reviewed Sep 28, 2023

View reviewed changes

Update src/dataworker/Dataworker.ts

a5b8324

Co-authored-by: Paul <[email protected]>

nicholaspai requested a review from pxrl September 28, 2023 19:25

mrice32 approved these changes Sep 28, 2023

View reviewed changes

mrice32 reviewed Sep 28, 2023

View reviewed changes

Merge branch 'master' into npai/transfer-threshold-removal

5dccd30

nicholaspai requested a review from mrice32 September 29, 2023 02:30

mrice32 approved these changes Sep 29, 2023

View reviewed changes

nicholaspai added 2 commits September 29, 2023 09:46

Merge branch 'master' into npai/transfer-threshold-removal

c6b4f99

Merge branch 'master' into npai/transfer-threshold-removal

b95a732

nicholaspai merged commit f7ea6fd into master Sep 29, 2023
2 checks passed

pxrl deleted the npai/transfer-threshold-removal branch September 29, 2023 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve(dataworker): Remove `transferThreshold` #937

improve(dataworker): Remove `transferThreshold` #937

nicholaspai commented Sep 13, 2023 •

edited

Loading

linear bot commented Sep 13, 2023

nicholaspai commented Sep 21, 2023

nicholaspai commented Sep 21, 2023

nicholaspai commented Sep 27, 2023

james-a-morris commented Sep 27, 2023

pxrl Sep 28, 2023

nicholaspai Sep 28, 2023

pxrl Sep 29, 2023 •

edited

Loading

nicholaspai Sep 29, 2023

nicholaspai Sep 29, 2023

pxrl Sep 28, 2023

nicholaspai Sep 28, 2023

pxrl Sep 29, 2023

mrice32 left a comment

mrice32 Sep 28, 2023

nicholaspai Sep 29, 2023

mrice32 Sep 28, 2023

nicholaspai Sep 29, 2023

mrice32 left a comment

improve(dataworker): Remove transferThreshold #937

improve(dataworker): Remove transferThreshold #937

Conversation

nicholaspai commented Sep 13, 2023 • edited Loading

linear bot commented Sep 13, 2023

nicholaspai commented Sep 21, 2023

nicholaspai commented Sep 21, 2023

nicholaspai commented Sep 27, 2023

james-a-morris commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxrl Sep 29, 2023 • 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

mrice32 left a 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

mrice32 left a comment

Choose a reason for hiding this comment

improve(dataworker): Remove `transferThreshold` #937

improve(dataworker): Remove `transferThreshold` #937

nicholaspai commented Sep 13, 2023 •

edited

Loading

pxrl Sep 29, 2023 •

edited

Loading