feat (Lending): deposit basics

[antoncoding]

Summary

• Deposit USDC into the Lending contract and create balance

Details

• Update to handleAdjustment: add manager check
• the deposit function simply convert token amount => 18 decimals, and update that value in Account.
• Add DecimalMath library that convert decimals for uint256
• in DecimalMathTest test: it needs to setup a "helper contract" to let the forge coverage work properly with internal library. (reference)

Todo

• decimal conversion for deposit
• Unit tests for deposit
• 100% coverage for deposit and convertDecimals

Checklist

Ensure you completed all of the steps below before submitting your pull request:

• Add natspec for all functions / parameters
• Ran forge snapshot
• Ran forge fmt
• Ran forge test
• Triage Slither issues, and post uncertain ones in the PR
• 100% test coverage on code changes

Slither Issues (Optional)

If you're unsure about a new issue reported by Slither, copy them here so others can verify as well.

[antoncoding]

@LondonCalamari @joshpwrk 2 things I want your opinion on:

1. I think having this PR first is gonna help with testing interest rate logic later, that's why i'm having this bare bone PR. do you think it's good to keep PR this size or have more stuff within 1 pr (like both deposit + withdraw if there're equivalently small )?

2. I know by definition, it's not "unit tests" anymore if I setup Account contract and use it to test the actual callback of the hook, but I think it is more convenient to do it this way. Do you guys think this should belong to integration test or unit test?

• I feel like it's clearer to live under integration test. but we might have another "more complex" integration test that includes real manager + real account + real asset in the future (after proto 1), so we will need to separate them somehow

[joshpwrk]

@LondonCalamari @joshpwrk 2 things I want your opinion on:

1. I think having this PR first is gonna help with testing interest rate logic later, that's why i'm having this bare bone PR. do you think it's good to keep PR this size or have more stuff within 1 pr (like both deposit + withdraw if there're equivalently small )?
2. I know by definition, it's not "unit tests" anymore if I setup Account contract and use it to test the actual callback of the hook, but I think it is more convenient to do it this way. Do you guys think this should belong to integration test or unit test?

• I feel like it's clearer to live under integration test. but we might have another "more complex" integration test that includes real manager + real account + real asset in the future (after proto 1), so we will need to separate them somehow

1. I really like the current size - I feel like once things get really busy in the middle of a sprint, any PR longer than this I'd end up missing all the details

2. I'm actually in favor of using Account but still keeping it in the unit-tests since the intent is to only test the behavior of the lending asset. I feel it will be quite hard to do useful unit-tests if we always limit them to only using mocks -> I think the intent matters more.

[LondonCalamari]

@LondonCalamari @joshpwrk 2 things I want your opinion on:

1. I think having this PR first is gonna help with testing interest rate logic later, that's why i'm having this bare bone PR. do you think it's good to keep PR this size or have more stuff within 1 pr (like both deposit + withdraw if there're equivalently small )?
2. I know by definition, it's not "unit tests" anymore if I setup Account contract and use it to test the actual callback of the hook, but I think it is more convenient to do it this way. Do you guys think this should belong to integration test or unit test?

• I feel like it's clearer to live under integration test. but we might have another "more complex" integration test that includes real manager + real account + real asset in the future (after proto 1), so we will need to separate them somehow

1. Agree with Josh here. I think these PR sizes are very digestible, its alot easier to review ones this size.
2. Unit test still makes sense here to me because the main intent of the tests is for Lending. Integration tests would be more for testing functionality across multiple contracts ?

[antoncoding]

@LondonCalamari @joshpwrk I added the DecimalMath library in this PR and make the forge coverage work with it (finally). I think it's reasonable to assume that other workstreams won't need to use this lib for now, cause both manager and liquidation we only need numbers from Account which is always in 1e18.
(Feel like only asset contracts maybe OptionAsset too will need it, because we have to deal with exchanging with ERC20s)

Let me know if this is a good idea to have it here for now, and merge to other workstream later after sprint 1.

[LondonCalamari]

@LondonCalamari @joshpwrk I added the DecimalMath library in this PR and make the forge coverage work with it (finally). I think it's reasonable to assume that other workstreams won't need to use this lib for now, cause both manager and liquidation we only need numbers from Account which is always in 1e18. (Feel like only asset contracts maybe OptionAsset too will need it, because we have to deal with exchanging with ERC20s)

Let me know if this is a good idea to have it here for now, and merge to other workstream later after sprint 1.

I think its okay to have it here for now and merge after sprint 1

[LondonCalamari]

Looks good