Skip to content

Conversation

@MaximusHaximus
Copy link
Contributor

Description

This PR implements 'complete' price support for Naga.

Setting the max price for a request

There are two ways SDK consumers can set the max price they want to pay for a request.

  1. Existing LitNodeClient behaviour methods (e.g. decrypt, executeJs and pkpSign) have a new optional argument, userMaxPrice: bigint. If a userMaxPrice is provided when calling those methods, it will be used to calculate how much the maxPrice should be for each node.
  2. SDK consumers can also configure default values for how much they want to pay for each type of request (SIGN, DECRYPTION, LIT ACTION) on the LitNodeClient by calling litNodeClient.setDefaultMaxPrice(product: keyof typeof PRODUCT_IDS, price: bigint). If the LitNodeClient is configured with a specific default max price for a particular product, and no userMaxPrice is provided when the user calls that product method, the default will be used instead.
  3. If no userMaxPrice is provided when calling behaviour methods on LitNodeClietn and no default has been set for that product, then the maxPrice for each node will be un-bound, and as a result the request cost will be whatever the cheapest subset of nodes that can execute the request charge at that time.
    • Tinny's LitNodeClient can now be configured via environment variable DEFAULT_MAX_PRICES, which is an array of strings that will be parsed as bigint.
  • If the userMaxPrice provided by the caller or set on the LitNodeClient is too low for current network conditions and your request cannot be fulfilled without spending more than userMaxPrice, the LitNodeClient will throw a MaxPriceTooLow error with context.

SessionSigs are no longer consumer-facing

  • Note that pricing differs depending on the product being used -- signing can have a different price than running a LIT action, for example.
  • As a result of this variability, we needed to create sessionSigs on the fly, because sessionSigs must include a maxPrice parameter for each node we are sending a request to, and the maxPrice will be different depending on the type of request being sent.
  • Because sessionSigs must be created on-the-fly, we have internalized their creation entirely. Instead of users passing sessionSigs to our behaviour methods, users pass an AuthenticationContext object, which contains the parameters to be used to generate session sigs. The LitNodeClient then uses the AuthenticationContext to create session sigs with the appropriate maxPrice for each node just before it sends the requests to the nodes.
  • Due to this change, getSessionSigs() is now a private method on the LitNodeClient (_getSessionSigs()). It is not expected for SDK consumers to need to create sessionSigs during typical usage from now on.

Details

  • Refactored tracking of node prices to use BigInts instead of numbers
  • Pricing data is always fetched for all product IDs -- there are no cases where we ever want pricing indexes to be different from call-to-call since the data returned is positional, not an object
  • Pricing data is stored in an array sorted by the DECRYPTION product, containing {url: string, prices: bigint[] } objects
  • Removed most SessionSigs related types, and replaced them with AuthenticationContext
  • Removed deprecated code from pkp-base and authClient
    • Replaced (deprecated) controllerAuthMethods,controllerSessionSigs and controllerAuthSig with just AuthenticationContext
    • I didn't add maxUserPrice to pkp-base (and objects that extend from it) behavioural methods. I am keen for feedback on whether we think being able to use setDefaultMaxPrice() on the LitNodeClient instance will be sufficient for usage of these objects. cc: @Ansonhkg @FedericoAmura
  • Added support for loading PriceFeed contract ABIs from Shiva TestNetwork configuration
  • Updated error logging for in tinny-operations so that we log VErrors appropriately when tests fail, both in-line where the test failed, and at the end when showing test failure results, and include VError.info() in printed output when it exists on the error that we catch.
  • Updated all local-tests to use the new AuthenticationContext pattern instead of using getSessionSigs().
  • I added explicit eslint-ignore pragmas to a few files where we have a lot of any datatypes. Files like interfaces, types, contracts-sdk are now "clean", so if you see errors in your IDE gutter in those files you can be confident your changes have introduced them :)
    • I removed timestamp2Date(), which had a dynamica require() of the date-and-time package. I couldn't find anywhere we use the function.
  • @FedericoAmura You are a saint -- nice work on killing the autogen-injected contracts-sdk code. I've enabled our import/order rule in that file now that it's not being dynamically built. This should help a lot with conflicts while merging/rebasing code in that file from here on out
  • I removed the interface for PricingInfo; nothing external needs to consume it, so keeping it out of our advertised interfaces makes it easier to change as we iterate without worrying about it being an API level breaking change.
  • The set of Nodes that requests are sent to (and the nodeSet provided to those nodes) is now dynamic; we use the required # of nodes for consensus that are also the cheapest priced nodes.
  • lit-auth-client's AuthProviders had some funky types/interfaces and inheritance that I unravelled, centered around the authenticate() abstract method. I removed the extends AuthenticateOptions which was a pass-through for an empty interface.
  • I added basic unit tests for get-max-prices-for-node-product
  • I removed the already-deprecated runOnTargetedNodes() method that we haven't used in quite some time; this allowed us to also remove getIpfsId() from LitNodeClient - I couldn't find anywhere other than runOnTargetedNodes() that it was actually used, and I would think that ipfsIds should be able to be computed client-side without running a LIT action anyway (right?)

Notes:

  1. Currently wrapped-keys tests, builds, and publishing is disabled. Wrapped-keys is a bit of an interesting case because we use sessionSigs to authenticate, and the backend doesn't yet support any Naga featureset networks.
  2. I removed the LitClientSessionManager interface; it was very sticky with SessionSigs, and I think it will be replaced entirely with our AuthManager pattern

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Ran all local-tests against lit-assets @ 60318791258d273df8209b912b386680d25d0df3)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

…sort, and retrieve prices for all product IDs in all cases

- We never want to only update one price since it costs us the same amount of RPC time, and getting a subset will be error prone since the indexes won't match our product IDs anymore
- Updated type signatures to be correct (the keys on the price map by node URL are arrays of prices) and added a type for ValidatorStruct with prices for simplicity
- Fixed some other eslint errors
- Simplified the building of the networkPriceMap
- Removed the networkPrices.arr, as it is unused
…om `getSessionSigs()`

- Pricing hash will be provided to the function as an argument.
- Add pass-through of `product` to `getPkpSessionSigs()` and `getLitActionSessionSigs()`, and update `getPkpSessionSigs` to provide `maxPricesBytNodeUrl` to `getSessionSigs()` call
…essionSigs()` and `getPkpSessionSigs()` with `getXXXAuthContext()` methods
…nalizing sessionSigs in LitNodeClient, lit-auth-client, and pkp-base

- Removed now-unused IPFS-hash-specific lit action rather than refactor the function to use authContext (it was marked deprecated already)
- Log error responses when errors encountered getting signing shares from nodes properly in lit-core handling
- Update `decryptFromJson` to use authContext
- Refactor lit-auth-client BaseProvider to no longer use sessionSigs
- Remove LitClientSessionManager interface, as it is parasitic and entwined with SessionSigs
- Remove controllerXXXXX (authsig authmods and sessionSigs) and replace with AuthContext in pkp-base
…ests to use authContext

- Log error stacks in tinny-operations
- Use `StaticJsonRpcProvider` in tinny-environment `getFunds()` so it works in nodeJS without throwing network not found error
- Export auth context helpers where sessionSig helpers used to be exported
- Disable raw authSig tests until Naga supports raw authSigs for encryption_sign endpoint
…ngleResponse` to use authContext and add it to `test.ts` for consistency w/ `tests.ts`
- Remove unused types & interfaces
- Add common AuthenticationContext type
- Remove nested `getSessionSigsProps` key on AuthContext used by pkp-base and use the same type there as on `lit-node-client-nodejs`
- Add eslint-disable-rule for `any` usages in interfaces.ts so that we can tell if we've introduced actual, new errors easily
- Update types usage in `lit-node-client-nodejs` appropriately
- Remove unused imports in `BaseProvider`
…kpEthers tests

- Also remove from pkp-walletconnect.spec.ts
- Make resourceAbilityRequests optional array on AuthenticationContext since we default it all over the place :(
…s in types.ts so we can easily see if we're introducing new errors
…on logic.

- Only return networkPrices as a sorted array of {url, prices[]}
- Updated tests to work with networkPrices array structure
- Eliminate some more eslint errors
…ar to configure lit node client that is used by Tinny tests

- Modify key from 'LA' to 'LIT_ACTION' on PRODUCT_IDS constant mapper for clarity
…parse undefined' if an attempt to run node commands has misconfigured maxNodeCount vs. threshold count
….minNodeCount and handle case where we have only claim data but no signed data (testUseValidLitActionCodeGeneratedSessionSigsToExecuteJsClaimKeys fixed)

- Also re-added missing realmId in call to `getNodesForRequest()` (git merge regression)
… so we can work from a clean file in contracts-sdk
Copy link
Collaborator

@Ansonhkg Ansonhkg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LFG

@Ansonhkg Ansonhkg merged commit 7c542f6 into feat/naga-fix-bls-wasm-cleanup Feb 3, 2025
3 of 4 checks passed
@Ansonhkg Ansonhkg deleted the feature/lit-3748-naga-user-set-maxprice branch February 3, 2025 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants