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

#2112 - add option to configure priority fees #2128

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

sebastianscatularo
Copy link
Collaborator

  • expose option to set priority fees percentile
  • expose function to compute priority fees with an alternative mechanism

Copy link

netlify bot commented May 23, 2024

Deploy Preview for wormhole-connect ready!

Name Link
🔨 Latest commit 5454fa9
🔍 Latest deploy log https://app.netlify.com/sites/wormhole-connect/deploys/6654d493613f9000089a29f9
😎 Deploy Preview https://deploy-preview-2128--wormhole-connect.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented May 23, 2024

Deploy Preview for wormhole-connect-mainnet ready!

Name Link
🔨 Latest commit 5454fa9
🔍 Latest deploy log https://app.netlify.com/sites/wormhole-connect-mainnet/deploys/6654d49351132300083aa7c7
😎 Deploy Preview https://deploy-preview-2128--wormhole-connect-mainnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sebastianscatularo sebastianscatularo marked this pull request as ready for review May 28, 2024 13:40
async function determinePriorityFee(
connection: Connection,
lockedWritableAccounts: PublicKey[] = [],
percentile: number,
customDeterminePriorityFee: (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see how it would be useful to an integrator because we're not providing the connection to this custom function, only the addresses. they can't do much with only addresses unless they're providing their own connection.

also, the way you're using it here is you're just setting the default value using it, rather than the final value. it will still get overridden by our own logic, which follows it in this function starting on line 115. so this doesn't seem like it will even work.

It seems to me we want to either:

  • let integrators completely override the determinePriorityFee function
  • or let them just provide a custom percentile and keep our logic the same

this code change is sort of a weird combination of the two.

I would keep things simple and go with option 2 for now. can we just remove this custom callback function, and only add the percentile number as a new config option?

if there's need for deeper customizability in the future we can revisit overriding determineComputeBudget later.

Choose a reason for hiding this comment

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

So this solution wont' work? I asked @barnjamin about this functionality but i'm having problems trying to clone this commit and installing it to my react app so i couldn't test it, and as i see it didn't merged to development brach yet right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good call. I’ll add it as a parameter. The idea is to allow integrators to use a different API, like what Helius provides for estimating priority fees, or any custom solution that someone might want to try, the limit is the skies 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to offer both: custom percentile, or custom determinePriorityFee function altogether.

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