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): introduce addComputeUnitInstructions #1091

Merged
merged 6 commits into from
Dec 20, 2024

Conversation

ryandgoulding
Copy link
Contributor

No description provided.

@ryandgoulding
Copy link
Contributor Author

not ready for review, just letting it go through CI pipeline as it is faster than doing so locally

@ryandgoulding ryandgoulding force-pushed the feat/solana_priority_fee branch 2 times, most recently from d1617ce to 90ab9db Compare December 2, 2024 14:00
@ryandgoulding ryandgoulding changed the title do not merge yet: solana priority fees solana priority fees Dec 2, 2024
@DanL0
Copy link
Contributor

DanL0 commented Dec 4, 2024

@ryandgoulding can you resolve conflict?

@ryandgoulding ryandgoulding force-pushed the feat/solana_priority_fee branch from 90ab9db to 0e8f6e0 Compare December 4, 2024 17:19
@ryandgoulding
Copy link
Contributor Author

@DanL0 done, can you take a look?

DanL0
DanL0 previously requested changes Dec 11, 2024
examples/oft-solana/tasks/solana/index.ts Show resolved Hide resolved
@nazreen
Copy link
Contributor

nazreen commented Dec 17, 2024

I tested locally and here are my results, using the time utility in the terminal to measure

task ran: createOFT

without calling addPerformanceInstructions

pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.02s user 0.34s system 4% cpu 53.281 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  1.99s user 0.35s system 4% cpu 52.599 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.04s user 0.37s system 4% cpu 53.974 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  1.83s user 0.32s system 5% cpu 40.226 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true   1.95s user 0.34s system 4% cpu 52.211 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true   2.02s user 0.34s system 4% cpu 53.015 total

with calling addPerformanceInstructions

pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.06s user 0.33s system 5% cpu 42.535 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.04s user 0.37s system 4% cpu 53.974 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.17s user 0.37s system 3% cpu 1:04.41 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.27s user 0.36s system 4% cpu 1:05.64 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.05s user 0.35s system 5% cpu 43.787 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  1.99s user 0.34s system 5% cpu 39.079 total

So overall it does improve transactions processing times

@ryandgoulding we can merge it, just need to fix the getFee interface change and rename the function

If you want me to help, I don't mind

@ryandgoulding
Copy link
Contributor Author

I tested locally and here are my results, using the time utility in the terminal to measure

task ran: createOFT

without calling addPerformanceInstructions

pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.02s user 0.34s system 4% cpu 53.281 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  1.99s user 0.35s system 4% cpu 52.599 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.04s user 0.37s system 4% cpu 53.974 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  1.83s user 0.32s system 5% cpu 40.226 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true   1.95s user 0.34s system 4% cpu 52.211 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true   2.02s user 0.34s system 4% cpu 53.015 total

with calling addPerformanceInstructions

pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.06s user 0.33s system 5% cpu 42.535 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.04s user 0.37s system 4% cpu 53.974 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.17s user 0.37s system 3% cpu 1:04.41 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.27s user 0.36s system 4% cpu 1:05.64 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  2.05s user 0.35s system 5% cpu 43.787 total
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id $PROGRAM_ID  true  1.99s user 0.34s system 5% cpu 39.079 total

So overall it does improve transactions processing times

@ryandgoulding we can merge it, just need to fix the getFee interface change and rename the function

If you want me to help, I don't mind

Sure, please feel free to take over the branch!

@nazreen
Copy link
Contributor

nazreen commented Dec 19, 2024

@ryandgoulding changes made

Summary:

  • renamed addPerformanceInstructions to addComputeUnitInstructions
  • removed computeUnitLimitScaleFactor as a param, but kept it as a local variable inside addComputeUnitInstructions
  • updated the getPrioritizationFee interface to make programId optional

I've Approved, but we should only merge after you also review

@nazreen nazreen self-requested a review December 19, 2024 07:23
@nazreen
Copy link
Contributor

nazreen commented Dec 19, 2024

Applied 'do not merge' as running into error when running the create task

Ran pnpm hardhat lz:oft:solana:create --eid 30168 --program-id <PROGRAM_ID> --compute-unit-price-scale-factor 2 --only-oft-store true

Got error

Message: Transaction simulation failed: Attempt to debit an account but found no record of a prior credit..

EDIT: was using mainnet eid accidentally

@nazreen
Copy link
Contributor

nazreen commented Dec 19, 2024

@ryandgoulding I've tested by running

pnpm hardhat lz:oft:solana:create --eid 40168 --program-id <PROGRAM_ID> --compute-unit-price-scale-factor 2 --only-oft-store true

and it works fine, on devnet

@nazreen
Copy link
Contributor

nazreen commented Dec 19, 2024

was surprised that checks here were failing so I created a PR based off of latest main that only has an empty commit

it's failing too - #1138

@nazreen nazreen changed the title solana priority fees feat(oft-solana): introduce addComputeUnitInstructions Dec 19, 2024
@nazreen
Copy link
Contributor

nazreen commented Dec 20, 2024

if after latest run of CI (after merging in main that has the fix), checks pass, Do Not Merge label should be removed.

@nazreen nazreen dismissed DanL0’s stale review December 20, 2024 18:59

reported error has been fixed by 8fbdfb1

reviewer is on PTO but we are receiving reports from partners running into difficulty landing transactions, which can be resolved by this PR getting merged

@nazreen nazreen merged commit d4850c3 into main Dec 20, 2024
7 checks passed
@nazreen nazreen deleted the feat/solana_priority_fee branch December 20, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants