Solana: full getValidatedFee flow [NONEVM-676] #405
Conversation
PabloMansanet
force-pushed
the
network_fees
branch
from
c8f2c08 to
89f683f
Compare
There was a problem hiding this comment.
I haven't reviewed the tests yet, but here are a few comments in the meantime. It mostly looks good, but there are some opportunities to simplify the passed-in accounts and iterations, as well as some places where more validations of the remaining_accounts may be beneficial.
PabloMansanet
changed the title
Implement network fee calculation Refactor and cleanup Update unit test values Add unit tests More unit tests Use account array for GetFee Fix all tests except token happy path Cleanup Use lookup table for fee calculation in ccip_send Cleanup Rebase fixes Fix lints Comment improvement Implement all fee calculations Fix unit tests Validate accounts Make fee billing config optional Address review comments Adjust test values for BPS Regenerate bindings and update tests
PabloMansanet
force-pushed
the
network_fees
branch
from
73aa951 to
a47ba8c
Compare
|
| message.validate(dest_chain, token_config)?; | ||
|
|
||
| let token = if message.fee_token == Pubkey::default() { | ||
| let fee_token = if message.fee_token == Pubkey::default() { |
There was a problem hiding this comment.
We should have a check here for Chain family right? And only run this when the chain family is EVM
| FEE_BILLING_TOKEN_CONFIG, | ||
| if mint.key() == Pubkey::default() { | ||
| native_mint::ID.as_ref() // pre-2022 WSOL | ||
| } else { | ||
| mint.key.as_ref() | ||
| }, |
There was a problem hiding this comment.
This is probably not necessary for tokenpools. Although this is how we solve for fee_token_config accounts, AFAIK it should never be the case that someone is sending native SOL through CCIP (@agusaldasoro or @aalu1418 to confirm, IMHO they should send WSOL in that case instead).
If my above statement holds, you can just require a non-zero mint and remove this if.
| ) -> Usd18Decimals { | ||
| // Sums up byte lengths of fixed message fields and dynamic message fields. | ||
| // Fixed message fields do account for the offset and length slot of the dynamic fields. | ||
| let data_availability_length_bytes = ANY_2_EVM_MESSAGE_FIXED_BYTES |
There was a problem hiding this comment.
Food for thought for upcoming PRs (no change required here), based on a chat with @agusaldasoro... The terminology any2evm is imported from the EVM contracts; should we rename it to solana2any or solana2evm in the solana contract?
| anotherUserATA solana.PublicKey | ||
| tokenlessUserATA solana.PublicKey | ||
| billingConfigPDA solana.PublicKey | ||
| perChainPerTokenConfigPDA solana.PublicKey |
There was a problem hiding this comment.
Given that this represents just one chain's config PDA for the particular token, please include it in the attribute name. That way we can later have stuff like wsol.evmConfigPDA or wsol.solanaConfigPDA for the different chains for a given token.
Feel free to merge as-is and change this in a separate PR if you prefer
Full implementation of ccip billing, with the exception of ExtraArgs (TODO left to that effect). The logic is extracted and refactored from FeeQuoter.sol.
Integration tests don't yet compare the total calculation with the EVM code. For now, all that's tested (between unit and integration tests) is that increasing the various variables that affect fees does indeed result in a difference in the output.
I feel like the best way to approach those tests will be to sample the EVM implementation for a large set of scenarios, and reproduce these scenarios in integration tests here to ensure the result is the same. I plan to add these after the holidays in a follow-up PR.