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

Add simulations & wagmi hooks #69

Closed
wants to merge 129 commits into from

Conversation

Rubilmax
Copy link
Collaborator

@Rubilmax Rubilmax commented Sep 10, 2024

Requests are faster because each of them is now independent (no need to fetch the config before fetching the rest of each market/vault). But deployless reads are not multicall-able, meaning they can't be batched in a single onchain RPC call via multicall.

This state is arguable and does not seems better out-of-the-box. However, here is why it is better:

  • requests can be batched with most RPC backends nowadays
  • requests are already actually batched using HTTP/2 natively via the browser (so there's no actual "delay" due to HTTP overhead)

So the only matter is the request count for RPC backends, which will be the cost paid by the RPC user in the end. It should stay relatively low considering that now, each entity costs at most 1 RPC request (either through the multicall = 0, or through a single deployless read = 1)

Fixes INTER-1016

@Rubilmax Rubilmax self-assigned this Sep 10, 2024
@Rubilmax Rubilmax force-pushed the feat/blue-sdk-viem-deployless-reads branch 6 times, most recently from 1c20476 to dc5c4b6 Compare September 10, 2024 16:20
Copy link
Contributor

@julien-devatom julien-devatom left a comment

Choose a reason for hiding this comment

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

can't we let the user choose the method to use? either with multicall or with deployless

packages/blue-sdk-viem/contracts/interfaces/IMorpho.sol Outdated Show resolved Hide resolved
@Rubilmax Rubilmax force-pushed the feat/blue-sdk-viem-deployless-reads branch 2 times, most recently from 774c8a2 to cf029fe Compare September 11, 2024 09:45
@Rubilmax Rubilmax force-pushed the feat/blue-sdk-viem-deployless-reads branch from cf029fe to cf2ca37 Compare September 11, 2024 11:13
julien-devatom
julien-devatom previously approved these changes Sep 11, 2024
Copy link
Contributor

@julien-devatom julien-devatom left a comment

Choose a reason for hiding this comment

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

Since we already know that deployless fails for mainnet weth, wdyt about always skipping deployless with a set of address? It can prevent known rpc fails

Else everything looks good

packages/blue-sdk-viem/src/fetch/Market.ts Outdated Show resolved Hide resolved
packages/blue-sdk-viem/test/Holding.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@julien-devatom julien-devatom left a comment

Choose a reason for hiding this comment

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

I don't have reviewed logic yet but few remarks from your Thursday presentation:

blue-sdk-viem-simulation should not depend on react imo. Ex if we want to use it in liquidation SDK. A solution would be to have a dedicated package for the hook useSimulationState (which is ok I think)

  • I think it's not necessary to have "viem" in the SDK name for a non primitive SDK like blue-sdk. underthehood, as soon as you want to use simulation, you use viem.

@Rubilmax Rubilmax closed this Oct 9, 2024
@Rubilmax Rubilmax deleted the feat/blue-sdk-viem-deployless-reads branch October 18, 2024 17:07
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.

3 participants