Skip to content
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

Fee estimate server side cache #4398

Merged
merged 7 commits into from
Jun 6, 2023
Merged

Conversation

just-a-node
Copy link
Collaborator

@just-a-node just-a-node commented Jun 1, 2023

Description

Minimal changes to enable optional caching on the estimateRelayerFee route. This can be extended to other routes if necessary. Opted not to intercept axiosGet requests in the Gelato calls as this requires changes in the utils package and would make removing the server dependency more complex later. This should suffice for reducing latency since most client calls will not use the optional params in SdkEstimateRelayerFeeParams and will hit the cache for subsequent requests to the same originDomain/destinationDomain.

Type of change

  • Docs change / dependency upgrade
  • Configuration / tooling changes
  • Refactoring
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Requires changes in customer code

High-level change(s) description - from the user's perspective

Related Issue(s)

Fixes #4223

Related pull request(s)

@sanchaymittal
Copy link
Collaborator

i think we should do that cache at the sdk code only, instead of adding it to server. Server should be purely a wrapper around the sdk.
@just-a-node

Copy link
Collaborator

@sanchaymittal sanchaymittal left a comment

Choose a reason for hiding this comment

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

best to keep cache at core-sdk, and keeping server wrapper to main functions

@just-a-node
Copy link
Collaborator Author

@sanchaymittal Wait, sdk-server is what's being hosted. We're going to have a separate wrapper package that mimics the client SDK interface and calls these server routes. So this cache is at the API side

@sanchaymittal
Copy link
Collaborator

From what I can recall from the last discussion, Is that we want to release under package @connext/sdk only with different versioning. So ideally we want to have cache being optional at core-sdk itself and wrapper at Server. And sdk turned into wrapper of api calls of server.

@just-a-node
Copy link
Collaborator Author

I think we have a similar understanding, but see structure in #4228.

What I was assuming is:

  • core SDK stays the same
  • sdk server stays basically the same, now it's hosted
  • new package wraps api calls to the server and it's published to @connext/sdk

@just-a-node
Copy link
Collaborator Author

The cache implemented here is the middleware described in #4223. This is caching at the API layer, not the service level of core sdk

sanchaymittal
sanchaymittal previously approved these changes Jun 3, 2023
rhlsthrm
rhlsthrm previously approved these changes Jun 5, 2023
Copy link
Collaborator

@rhlsthrm rhlsthrm left a comment

Choose a reason for hiding this comment

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

Nice! I guess we'd need ops setup for this.

@just-a-node
Copy link
Collaborator Author

Yes for redis and the right configs to feed server

@just-a-node just-a-node dismissed stale reviews from rhlsthrm and sanchaymittal via 2cb8743 June 5, 2023 19:47
@just-a-node just-a-node merged commit 0c29bb4 into main Jun 6, 2023
@just-a-node just-a-node deleted the fee-estimate-server-side-cache branch June 6, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Host sdk-server on AWS
3 participants