Oracle-based fee charging

[IaroslavMazur]

Closes #47. Depends on #174.

• feat(lk+mi): Oracle-based fee charging
• test(lk+mi): Oracle-based fee charging
• test: Added Bankrun Programs & Accounts
• test: Fetch account data from Devnet
• refactor(lk+mi): constants
• chore: remove the unused dependencies

[andreivladbrg]

@IaroslavMazur before I proceed with a full review, I have a quick question

why is the WITHDRAWAL_FEE_USD defined as a constant and not configurable?

[IaroslavMazur]

why is the WITHDRAWAL_FEE_USD defined as a constant and not configurable?

Because at this point, we don't have a way of modifying it (by an admin) after the program deployment.

This will be implemented in #171.

[andreivladbrg]

hmm, why not implement it from the beginning and do it as a 2-step process? 😆

[IaroslavMazur]

hmm, why not implement it from the beginning and do it as a 2-step process? 😆

Fee changing wasn't a part of our product offering for v1, as far as I remember.

[andreivladbrg]

Fee changing wasn't a part of our product offering for v1, as far as I remember.

what's v1?

we initially used a constant (for the lamports amount) due to not having chainlink available - but now, we should have the USD fee editable

now nvm, we'll implement it later then

[IaroslavMazur]

what's v1?

The first official release of SolSab.

we should have the USD fee editable

This subject was raised during the EthCC All-Hands - and, as far as I remember, the consensus was that it's not a must for V1.

I have no issue with implementing it now. In fact, that's what I envisioned for us in the nearest future, as well. I just explained why it was not a part of this PR.

[andreivladbrg]

This subject was raised during the EthCC All-Hands - and, as far as I remember, the consensus was that it's not a must for V1.

is it? then, i may have been remembered incorrectly - mb

[andreivladbrg]

left some comments on the lockup implementation — the same feedback can be applied to merkle_instant

i haven't reviewed the tests yet, as we'll first need to rebase from PRB's PR, which will of course cause conflicts
but there's one thing that caught my attention:

you're using the actual devnet address for the chainlink oracle
these are fork tests, not unit ones - we should declare some mocks that return various values, see this

or maybe chainlink already provides some test utils

[IaroslavMazur]

we should declare some mocks that return various values

For this,

• either Anchor would have to provide us with the ability to create mocks (which they don't currently offer, as far as I can tell)
• or we'd need to manually re-implement the Chainlink programs - and deploy them to Testnet/Devnet, where we want to test SolSab.

Are you suggesting we do the latter? And do it before the audit?

[andreivladbrg]

For this,

• either Anchor would have to provide us with the ability to create mocks (which they don't currently offer, as far as I can tell)
• or we'd need to manually re-implement the Chainlink programs - and deploy them to Testnet/Devnet, where we want to test SolSab.

asked Claude and it should be doable without having a very complex system
could you rebase first please? i can give it a try later today
if it turns out to be too complicated, let's stick with the devnet approach

[IaroslavMazur]

@andreivladbrg, rebased the PR.

Note that we still need to double-check the generic branch of the following match:

let fee_in_lamports: u64 = match oracle_decimals {
        8 => {
            // If the oracle decimals are 8, calculate the fee.
            CLAIM_FEE_USD * LAMPORTS_PER_SOL / price
        }
        decimals => {
            // Otherwise, adjust the calculation to account for the oracle decimals.
            CLAIM_FEE_USD * 10_u64.pow(10 + decimals as u32) / price
        }
    };

Also, I'm not sure why CI is failing. Will check it tomorrow.

[andreivladbrg]

will leave comments sequentially

[andreivladbrg]

Note that we still need to double-check the generic branch of the following match:

fixed in ed22eb1

[andreivladbrg]

@IaroslavMazur does the Treasury naming make sense anymore?

i believe we can rename it to comptroller (like in EVM)

wdyt?

[IaroslavMazur]

i believe we can rename it to comptroller (like in EVM)

Now that we have both the fee_collector and the Chainlink program addresses in there, Treasury does seem off-point.

"Comptroller" sounds good!

[andreivladbrg]

@IaroslavMazur left few more comments, i'll address them (already made changes locally)

[andreivladbrg]

Now that we have both the fee_collector and the Chainlink program addresses in there, Treasury does seem off-point.

"Comptroller" sounds good!

thanks

[IaroslavMazur]

@IaroslavMazur left few more comments, i'll address them (already made changes locally)

Awesome, thank you!

[andreivladbrg]

@IaroslavMazur pushed 2 more commits: 68e12c7 3b2504d

if they look good, could you please mark the above comments as resolved?

[IaroslavMazur]

The changes to the program code look good, in general.

Left some feedback.

[andreivladbrg]

left few comments more

[andreivladbrg]

@IaroslavMazur I did it 🎉 I finally managed to mock Chainlink!

I'd recommend revisiting coverage later with different mock scenarios.

Please review: cc02f36

[IaroslavMazur]

Very well done! Without the requests to the Solana Devnet & the Chainlink/Hermes oracles, the tests are truly flying now! 👏

Leaving some feedback below, along with a little cleaning in bc64a55.

[IaroslavMazur]

@andreivladbrg, squashed the commits from this branch - and rebased them onto main

[andreivladbrg]

Let's merge this

[IaroslavMazur]

Rebased the branch again.

@andreivladbrg, feel free to merge if it lgty.