Fujicracy / fuji-v2

Cross-chain money market aggregator
https://fuji-v2-frontend.vercel.app
15 stars 10 forks source link

Ironbank lending provider #234

Closed pedrovalido closed 1 year ago

pedrovalido commented 1 year ago

Adding IronBank as Lending provider. Everything is pretty straightforward as it was pretty similar with compound (using the same interfaces and library for balances). The only issue was on optimism and is described below. Looks like forks of compound and foundry do not work well together...

pedrovalido commented 1 year ago
Screenshot 2023-01-09 at 13 51 17

@DaigaroCota @brozorec while I was testing ironbank for optimism, I might have discovered the same limitation we had with the delegate calls. This time on calculating the balances on the acrobatic way. This way calculates the blockDelta (block.number - accrualBlockNumberPrior). The block.number is significantly lower than the accrualBlockNumber returned by the cToken (from IronBank) which leads to a underflow error (negative number). I marked it ready for review because the exact same thing is working for mainnet and I think this is a limitation of the optimism fork on foundry

0xdcota commented 1 year ago

@DaigaroCota @brozorec while I was testing ironbank for optimism, I might have discovered the same limitation we had with the delegate calls. This time on calculating the balances on the acrobatic way. This way calculates the blockDelta (block.number - accrualBlockNumberPrior). The block.number is significantly lower than the accrualBlockNumber returned by the cToken (from IronBank) which leads to a underflow error (negative number). I marked it ready for review because the exact same thing is working for mainnet and I think this is a limitation of the optimism fork on foundry

Hey @pedrovalido , just because this is optimism, I want to ensure what "block.number" is being returned. I think optimism has this capability on querying the ethereum block number and the optimistic "batch" (equivalent to block number). If a block number is significantly lower than the other; then you are probably dealing with this problem. See the number sequence of the image below.

image

pedrovalido commented 1 year ago

@DaigaroCota @brozorec while I was testing ironbank for optimism, I might have discovered the same limitation we had with the delegate calls. This time on calculating the balances on the acrobatic way. This way calculates the blockDelta (block.number - accrualBlockNumberPrior). The block.number is significantly lower than the accrualBlockNumber returned by the cToken (from IronBank) which leads to a underflow error (negative number). I marked it ready for review because the exact same thing is working for mainnet and I think this is a limitation of the optimism fork on foundry

Hey @pedrovalido , just because this is optimism, I want to ensure what "block.number" is being returned. I think optimism has this capability on querying the ethereum block number and the optimistic "batch" (equivalent to block number). If a block number is significantly lower than the other; then you are probably dealing with this problem. See the number sequence of the image below.

image

You're right. Seems like the block.number (inside the libCompoundV2) is returning the batch index instead of the L1 block number (which is being returned by accrualBlockNumber). From the optimism docs, they recommend using block.timestamp. After a quick search, I found out the way they're calculating the borrowBalanceCurrent in Optimism vs Ethereum is different. On ethereum they're using block.number and on optimism they're using block.timestamp. By changing the block.number to block.timestamp inside the LibCompoundV2 contract, the tests start passing! I've implemented a LibIronBankOptimism to reflect this changes and extra testing to make sure this changes are correct (fuzz testing acrobatic way of calculating balances on optimism). Thanks for noticing this :) @DaigaroCota