feat: allow any spl paid token flag

[Yolley]

Add allow_any_spl_paid_token flag that will bypass check (and takes precedence) for whether token is in allowed_spl_paid_tokens when checking whether transaction is paid.

For our use case we need to allow to use any SPL token for payment - the price of the token will be checked by the oracle. An alternative would be to allow allowed_spl_paid_tokens be empty, but I think that having this flag set explicitly makes more sense for backwards compatibility and security reasons.

[Yolley]

@dev-jodee @amilz please take a look, I can't assign reviewers.

[dev-jodee]

Small changes and you'll most likely need to rebase on release/pre-release-main-branch since there's been a lot of changes :)

[dev-jodee]

Can we use mocked RPC client here for unit testing we don't want to require the local validator to be running.

[Yolley]

Hm, I just used the same pattern with client initialisation as in other tests, will take a look.

[dev-jodee]

Hey, yeah might be the older version / so many changes have been happening, but here's the function to mock it https://github.com/solana-foundation/kora/pull/159/files#diff-e2aa8bf42a1b7b8111e7a9bc6b7990fa5efad819e5aa67fcdd2dbfed1ad79f53R93

[Yolley]

I see, thank you, updated the branch, please check.

[dev-jodee]

Use the RpcMockBuilder, it still points to a local url

[amilz]

Thanks for the PR @Yolley -- a couple of thoughts.

1. Unified SplTokenPaymentConfig

Rather than two potentially conflicting fields in the config, could we do a single one that accepts an allowlist or ALL? I think it'd be a cleaner devex and reduce risk of conflicting configs.

Something like this:

  // In config.rs
  pub enum SplTokenPaymentConfig {
      All,                    // Accept any SPL token
      Allowlist(Vec<String>), // Accept only specific tokens
  }

  impl Default for SplTokenPaymentConfig {
      fn default() -> Self {
          SplTokenPaymentConfig::Allowlist(vec![])
      }
  }

  // In ValidationConfig
  pub struct ValidationConfig {
      // ... other fields ...
      pub allowed_spl_paid_tokens: SplTokenPaymentConfig,
  }

I think this change might require a tweak to crates/lib/src/validator/config_validator.rs to skip check if All. Could even add a warning if all is enabled: "Warning allowed for payments"

2. Test coverage--would you mind adding a test or todo to make sure we throw if our oracle does not find a token.

[Yolley]

Hey, @amilz I don't think that making allowed_spl_paid_tokens a dynamic type field (either constant "ALL" or a list of values) in toml will be cleaner - seems like an anti pattern to have a field of different types. An alternative could also be to have magic "all" value in the allowed_spl_paid_tokens list, but then it also seems weird to mix real pubkeys and some magic values here.

I guess we could also make allowed_spl_paid_tokens an object in toml with two fields - mode and addresses or something. But then it will be backwards incompatible with how it's managed right now.

So in my opinion having 2 fields with one taking precedence over the other could be cleaner.

[Yolley]

@amilz I switched to the proposed solution - whilte I am not a fan of it, it's not necessarily bad. Also added a test for jup oracle for when it does not return price for a token. Please take look.

[Yolley]

@dev-jodee @amilz please take a look, with updates coming to the repo every couple of hours it becomes tedious to rebase this branch. 🌚

[dev-jodee]

@amilz I switched to the proposed solution - whilte I am not a fan of it, it's not necessarily bad. Also added a test for jup oracle for when it does not return price for a token. Please take look.

Hey @Yolley, sorry we've been crunching out features, the pr itself seems fine, I'm just trying to think of a case where a node operator would be willing to allow any spl tokens, with Solana having many 100s of thousands of tokens created every week or so, this seems extremely dangerous, what use case did you have in mind that would warrant allowing all spl tokens as payment?

[Yolley]

@amilz I switched to the proposed solution - whilte I am not a fan of it, it's not necessarily bad. Also added a test for jup oracle for when it does not return price for a token. Please take look.

Hey @Yolley, sorry we've been crunching out features, the pr itself seems fine, I'm just trying to think of a case where a node operator would be willing to allow any spl tokens, with Solana having many 100s of thousands of tokens created every week or so, this seems extremely dangerous, what use case did you have in mind that would warrant allowing all spl tokens as payment?

Hey, our use-case is to use fee relayer when claiming an Airdrop - so sol/rent fees are covered by the relayer, and user pays from their allocation. And an Airdrop can be created for any token as platform is permissionles.

Another use case I'd say could be for token-swaps? Like on a DEX - in case user does not have enough sol to cover processing fees, so the platform allows to pay with swapped tokens.

I don't see how it's "extremely" dangerous tho - we still check for the token price and make sure that the fees are covered according to pricing model configuration. I guess there could be an issue with prices changing a lot, but I don't see a clear fix for this right - I guess we could use a mix of volume/orderbook state or something, but it also depends on what Oracle can provide.

[Yolley]

@dev-jodee @amilz pretty please, I am tired of rebasing the branch.

[dev-jodee]

@dev-jodee @amilz pretty please, I am tired of rebasing the branch.

Think the pr makes sense, sorry for the recurring rebase, branch is actively being worked on, finally merged the huge pre-release PR into main, if you rebase against main (might be easier to make a new branch from main and cherry-pick your commit into it), I can do final review and merge

[Yolley]

@dev-jodee Hey, I rebased from main, also aded an additional test to toml_mock, please check and let's finally get this merged.

[dev-jodee]

Alright just a couple of nits for clarity and then we can merge :)

[dev-jodee]

Use the RpcMockBuilder, it still points to a local url

[amilz]

thx @Yolley
@dev-jodee do we need to do anything here to protect infinite loop when usesr calls initialize_atas ? probably would give me most peace of mind if we had a test init_ata_gracefully_does_nothing_if_all_tokens_enabled
also, wdyt about adding a warning on kora config validate: warning that all tokens are accepted--node may experience volatility, etc.

[Yolley]

Hey, @amilz I've added a test test_initialize_atas_when_all_tokens_are_allowed, also regarding warning - even if user passes some tokens, node can still experience volatility if they pass volatile tokens, does it really make sense to issue another warning here, given that the configuration to enable all tokens is explicit?