Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(oft-solana): add fallback for when getSimulationComputeUnits fails #1185

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nazreen
Copy link
Contributor

@nazreen nazreen commented Jan 22, 2025

Motivation

We've found that in times of network congestion, the RPC method that supports the getSimulationComputeUnits function would fail, resulting in scripts being stuck as every transaction currently relies on getSimulationComputeUnits. This blocks the deployment of Solana OFTs.

Solution

Given that as part of a Solana OFT deployment, we are executing a standard set of instructions, we can introduce a fallback by way of hardcoded CU estimates for each transaction type. The following is introduced into examples/oft-solana/tasks/solana/index.ts:

export enum TransactionType {
    CreateToken = 'CreateToken',
    CreateMultisig = 'CreateMultisig',
    InitOft = 'InitOft',
    SetAuthority = 'SetAuthority',
    InitConfig = 'InitConfig',
    SendOFT = 'SendOFT',
}

const TransactionCuEstimates: Record<TransactionType, number> = {
    // for the sample values, they are: devnet, mainnet
    [TransactionType.CreateToken]: 125_000, // actual sample: (59073, 123539), 55785 (more volatile as it has CPI to Metaplex)
    [TransactionType.CreateMultisig]: 5_000, // actual sample: 3,230
    [TransactionType.InitOft]: 70_000, // actual sample: 59207, 65198 (note: this is the only transaction that createOFTAdapter does)
    [TransactionType.SetAuthority]: 8_000, // actual sample: 6424, 6472
    [TransactionType.InitConfig]: 42_000, // actual sample: 33157, 40657
    [TransactionType.SendOFT]: 230_000, // actual sample: 217,784
}

We use the fallback in getComputeUnitPriceAndLimit:

...
export const getComputeUnitPriceAndLimit = async (
    connection: Connection,
    ixs: Instruction[],
    wallet: KeypairSigner,
    lookupTableAccount: AddressLookupTableAccount,
    transactionType: TransactionType
) => {
    const { averageFeeExcludingZeros } = await getFee(connection)
    const priorityFee = Math.round(averageFeeExcludingZeros)
    const computeUnitPrice = BigInt(priorityFee)

    let computeUnits

    try {
        computeUnits = await getSimulationComputeUnits(
            connection,
            ixs.map((ix) => toWeb3JsInstruction(ix)),
            toWeb3JsPublicKey(wallet.publicKey),
            [lookupTableAccount]
        )
    } catch (e) {
        console.error(`Error retrieving simulations compute units from RPC:`, e)
        console.log(
            `Falling back to hardcoded estimate for ${transactionType}: ${TransactionCuEstimates[transactionType]} CUs`
        )
        computeUnits = TransactionCuEstimates[transactionType]
    }
    ...

Notes:

  • The transaction with the most volatile CU count is createToken as it has CPIs into the Metaplex program. More basic transactions like setAuthority have more stable CU count.
  • Estimates may need to be updated if they are found to be insufficient.
  • Based on errors reported recently, getSimulationComputeUnits (to estimate CU limit) is the one failing and not getRecentPrioritizationFees (rpc method to estimate CU price)

Testing

To test, init a new repo with the following:

LAYERZERO_EXAMPLES_REPOSITORY_REF=#nazreen/oft-solana/get-simulation-cu-constants LZ_ENABLE_SOLANA_OFT_EXAMPLE=1 npx create-lz-oapp@latest

To simulate the getSimulationComputeUnits method failing in times of RPC congestion, make the following temporary edit to examples/oft-solana/tasks/solana/index.ts:

    try {
       throw 'Simulating Error' // <----- add this line
        computeUnits = await getSimulationComputeUnits(
            connection,
            ixs.map((ix) => toWeb3JsInstruction(ix)),
            toWeb3JsPublicKey(wallet.publicKey),
            [lookupTableAccount]
        )

Then, run any of the Solana scripts, such as

pnpm hardhat lz:oft:solana:create --eid 40168 --program-id <PROGRAM_ID> --only-oft-store true --amount 10000000000

and observe the logs

@nazreen nazreen changed the title Nazreen/oft solana/get simulation cu constants feat(oft-solana): add fallback for when getSimulationComputeUnits fails Jan 22, 2025
@nazreen nazreen requested review from DanL0 and ryandgoulding and removed request for DanL0 January 22, 2025 10:05
@nazreen
Copy link
Contributor Author

nazreen commented Jan 22, 2025

Check failing due to unrelated test:

PASS test/connection/factory.test.ts
@layerzerolabs/test-devtools-ton:test: FAIL test/node.spec.ts (10.882 s)
@layerzerolabs/test-devtools-ton:test:   TON local node
@layerzerolabs/test-devtools-ton:test:     ✕ should be available (10095 ms)
@layerzerolabs/test-devtools-ton:test: 
@layerzerolabs/test-devtools-ton:test:   ● TON local node › should be available
@layerzerolabs/test-devtools-ton:test: 
@layerzerolabs/test-devtools-ton:test:     expect(received).resolves.toEqual()
@layerzerolabs/test-devtools-ton:test: 
@layerzerolabs/test-devtools-ton:test:     Received promise rejected instead of resolved
@layerzerolabs/test-devtools-ton:test:     Rejected to value: [Error: Request failed with status code 504]
@layerzerolabs/test-devtools-ton:test: 
@layerzerolabs/test-devtools-ton:test:        7 |         const client = new TonClient({ endpoint })
@layerzerolabs/test-devtools-ton:test:        8 |
@layerzerolabs/test-devtools-ton:test:     >  9 |         await expect(client.getMasterchainInfo()).resolves.toEqual({
@layerzerolabs/test-devtools-ton:test:          |               ^
@layerzerolabs/test-devtools-ton:test:       10 |             workchain: -1,
@layerzerolabs/test-devtools-ton:test:       11 |             initSeqno: 0,
@layerzerolabs/test-devtools-ton:test:       12 |             latestSeqno: expect.any(Number),
@layerzerolabs/test-devtools-ton:test: 
@layerzerolabs/test-devtools-ton:test:       at expect (../../node_modules/.pnpm/[email protected]/node_modules/expect/build/index.js:113:15)
@layerzerolabs/test-devtools-ton:test:       at Object.expect (test/node.spec.ts:9:15)
@layerzerolabs/test-devtools-ton:test: 
@layerzerolabs/test-devtools-ton:test: Test Suites: 1 failed, 1 total
@layerzerolabs/test-devtools-ton:test: Tests:       1 failed, 1 total
@layerzerolabs/test-devtools-ton:test: Snapshots:   0 total
@layerzerolabs/test-devtools-ton:test: Time:        10.981 s

@nazreen nazreen requested a review from DanL0 January 22, 2025 10:13
@nazreen
Copy link
Contributor Author

nazreen commented Jan 23, 2025

checks are now passing

@ryandgoulding
Copy link
Contributor

Great work! Is this ready for review?

@nazreen nazreen marked this pull request as ready for review January 23, 2025 20:18
@nazreen
Copy link
Contributor Author

nazreen commented Jan 23, 2025

Great work! Is this ready for review?

Yes! Sorry, forgot it was created as a draft.

Copy link
Contributor

@ItsAdel ItsAdel left a comment

Choose a reason for hiding this comment

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

left some big picture suggestions

SendOFT = 'SendOFT',
}

const TransactionCuEstimates: Record<TransactionType, number> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

A few thoughts here:

I see we are overshooting the estimate for a lot of these Transaction Types, would we need to warn the users about the potential of overpaying ?

Another point here is that the estimates might not reflect the network CU usage overtime. In a few months time if everyone decides to use Solana, these values would need to be manually changed. Are we okay with making that manual change to keep the example upto date?

)
let computeUnits

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

thoughts on trying more than once before defaulting to estimates?

potentially using exponential backoff and retry and a couple times before defaulting

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: try/catch can just be condensed to

const computeUnits = await getSimulationComputeUnits(
  connection,
  ixs.map((ix) => toWeb3JsInstruction(ix)),
  toWeb3JsPublicKey(wallet.publicKey),
  [lookupTableAccount]
).catch(e => {
  console.error(`Error retrieving simulation compute units from RPC:`, e);
  console.log(
    `Falling back to hardcoded estimate for ${transactionType}: ${TransactionCuEstimates[transactionType]} CUs`
  );
  return TransactionCuEstimates[transactionType];
});

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