Update SOR buffers to use unwrapRate from erc4626 tokens #1353

brunoguerios · 2024-12-17T17:16:44Z

changeset-bot · 2024-12-17T17:16:48Z

🦋 Changeset detected

Latest commit: 31f0cd7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
backend	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

brunoguerios · 2024-12-18T13:19:32Z

modules/sources/contracts/v3/fetch-unwrap-rates.ts

+
+    // Convert the results to floats
+    const formattedResults = Object.fromEntries(
+        Object.entries(results).map(([key, value]) => [key, parseFloat(formatEther(value))]),


I'd rather not cast them to floats to prevent losing precision on the rate. Is there an issue on using strings?
I can update them on my end, just let me know if you think we might have an issue with that change.

hey, sure, whatever works. the only potential problem is that DB always stores floats, so breaking the standard will be confusing. would it make sense to change other values to keep evm scaling as well? eg: price rates from rate providers?

I'd say we should change all rates to strings 👍
We're doing our best to get balancer-maths and SOR to 100% match against on-chain results, so if we're able to avoid losing precision, I'd rather go that way.
Is there any downside to storing strings other than having to migrate everything?

i'd expect the same goes for token balances? totally agree on data correctness, though the decision, made ages ago, runs deep, impacting things like the subgraph. SOR is a pretty good reason to have evm values. I think we can start adding native values without much or tiny impact on existing code. tagging @franzns for thoughts.

Yeah, we could have a separate issue to keep track of this refactor, but I'd consider it low priority at the moment. Anyway, my suggestion is to already make new ones string/evm scale and even refactor as we touch them on other PRs, rather than keep creating floats and refactor everything at once later.
But that's my opinion.
Totally fine if you guys disagree and prefer to keep the standard (floats) for now and refactor all at once in a single PR.

Just so we have it registered here as well -> I don't think we need to store amounts in evm scale, I just think we need to avoid floats and store human scale as strings.

doesn't convertion from evm to human scale loose precision already?

not necessarily - as long as we store as strings it should be ok - for example:

evm scale (18 decimals): 12345678901234567890

human scale (string): 12.345678901234567890

human scale (float): 12.34567890123457

Ps: I don't remember exactly how many decimals float support, but it will eventually truncate

btw - parseFloat does lose precision, but that's why we shouldn't use it 🙃
we should stick with formatUnits, formatEther, parseUnits and parseEther to manipulate these values

yes, exactly. good summary, thanks!

brunoguerios · 2024-12-18T13:31:07Z

modules/sources/contracts/v3/fetch-unwrap-rates.ts

+ */
+export const fetchUnwrapRates = async (addresses: string[], chain: Chain) => {
+    const caller = new Multicaller3Viem(chain, MinimalErc4626Abi);
+    addresses.forEach((address) => caller.call(address, address, 'convertToAssets', [parseEther('1')]));


@johngrantuk - I'd like to discuss this line with you.
I decided to use 1e18 as input to convertToAssets so we always get the equivalent of 18 decimals unwrapRate disregard of the original token decimals (unless I'm missing something 🤔).
This should make it easier to handle any math regarding rates, right?
I know you took a different approach on Paraswap PR, so it would be nice to hear your opinion.

~~If you do it that way you will then have to scale the rate to the raw decimals when passing it as rate for a Buffer pool. That way the maths will work.~~

@brunoguerios, some useful test info:
The Gnosis pool, 0x272d6be442e30d7c87390edeb9b96f1e84cecd8d uses a rate provider that is nested. So unwrap rate does not equal rate between aave wsteth and eth. This particular case the rate provider accounts for growth of wsteth in terms of weth and the additional aave yield. This highlighted that rateProvider can not be used for buffer wrap/unwrap which instead should use erc4626 rate.

Mainnet Pool, 0x5dd88b3aa3143173eb26552923922bdf33f50949 has an ERC4626 token with 18 decimals that uses a 6 decimal underlying.
Note for maths: Instead of manually adding support for each ERC4626 implementation (e.g. stata with Ray maths) we always use an 18 decimal scaled rate and do 18 decimal maths to convert. We may end up loosing 100% accuracy but thats deemed acceptable.

So, let me check if I got this right:

balancer maths expect rate to be passed in with decimals equals to the erc4626 (wrapped) token

internally balancer maths always scale that rate to 18 decimals (possibly losing precision on RAY math implementations)

Ps: thanks for the test info - I'll create some tests to make sure the SOR is working as expected

@brunoguerios - sorry I read this wrong! Always using 1e18 for convertToAssets is correct. You should just be able to pass the returned rate as the buffer rate to the maths and it will work.

This would be so much easier if everyone kept consistency between wrapped/underlying decimals. This one case of 18 decimals wrapped and 6 decimals underlying will require quite a big change 🙄

brunoguerios · 2024-12-20T14:42:56Z

modules/actions/pool/v3/upsert-pools.ts

+            await prisma.prismaToken.upsert({
+                where: {
+                    address_chain: {
+                        address: token.address,
+                        chain,
+                    },
+                },
+                create: {
+                    ...token,
+                },
+                update: {
+                    ...token,
+                },
+            });


This feels a bit odd, so it would be nice to get feedback on how to upsert token data

true, it could be be shorter:

Suggested change

await prisma.prismaToken.upsert({

where: {

address_chain: {

address: token.address,

chain,

},

},

create: {

...token,

},

update: {

...token,

},

});

await prisma.prismaToken.upsert({

where: {

address_chain: {

address: token.address,

chain,

},

},

create: token,

update: token,

});

nice, thanks!
the code did change a lot since then, but it's nice to know how it could be better 😊

…buffers # Conflicts: # modules/subgraphs/balancer-subgraph/generated/balancer-subgraph-types.ts

brunoguerios requested review from gmbronco and franzns December 17, 2024 17:16

brunoguerios self-assigned this Dec 17, 2024

brunoguerios commented Dec 18, 2024

View reviewed changes

Update SOR buffers to use unwrapRate from erc4626 tokens

5ba5c15

brunoguerios force-pushed the sor-unwrap-rate-buffers branch from 36d2c61 to 5ba5c15 Compare December 20, 2024 14:39

brunoguerios commented Dec 20, 2024

View reviewed changes

franzns force-pushed the sor-unwrap-rate-buffers branch from c586e7a to 5cf9904 Compare December 20, 2024 16:05

brunoguerios force-pushed the sor-unwrap-rate-buffers branch from 5cf9904 to 5ba5c15 Compare December 20, 2024 16:07

brunoguerios added 4 commits December 20, 2024 16:27

Move unwrap rate sync to its own process

9ab3d40

Fix comment

36f83ab

Revert unnecessary changes

103ea14

Merge remote-tracking branch 'origin/v3-canary' into sor-unwrap-rate-…

31f0cd7

…buffers # Conflicts: # modules/subgraphs/balancer-subgraph/generated/balancer-subgraph-types.ts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update SOR buffers to use unwrapRate from erc4626 tokens #1353

Update SOR buffers to use unwrapRate from erc4626 tokens #1353

brunoguerios commented Dec 17, 2024

changeset-bot bot commented Dec 17, 2024 •

edited

Loading

brunoguerios Dec 18, 2024

gmbronco Dec 18, 2024

brunoguerios Dec 18, 2024

gmbronco Dec 18, 2024

brunoguerios Dec 19, 2024

brunoguerios Dec 19, 2024

gmbronco Dec 20, 2024

brunoguerios Dec 20, 2024

brunoguerios Dec 20, 2024 •

edited

Loading

gmbronco Dec 20, 2024

brunoguerios Dec 18, 2024

johngrantuk Dec 18, 2024 •

edited

Loading

johngrantuk Dec 18, 2024

brunoguerios Dec 18, 2024

johngrantuk Dec 18, 2024

brunoguerios Dec 19, 2024

brunoguerios Dec 20, 2024

gmbronco Dec 20, 2024

brunoguerios Dec 20, 2024

Update SOR buffers to use unwrapRate from erc4626 tokens #1353

Are you sure you want to change the base?

Update SOR buffers to use unwrapRate from erc4626 tokens #1353

Conversation

brunoguerios commented Dec 17, 2024

changeset-bot bot commented Dec 17, 2024 • edited Loading

🦋 Changeset detected

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoguerios Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngrantuk Dec 18, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Dec 17, 2024 •

edited

Loading

brunoguerios Dec 20, 2024 •

edited

Loading

johngrantuk Dec 18, 2024 •

edited

Loading