search

metaplex-foundation / sugar

Candy Machine Rust CLI.
Apache License 2.0
203 stars 116 forks source link

Add priority fees #472

Closed blockchain-bros closed 6 months ago

blockchain-bros commented 6 months ago

This adds priority fees to all transactions. It should be verified that it works (ref discussion on Discord).

MarkSackerberg commented 6 months ago

Hey @blockchain-bros, thank you very much for the PR! Regarding the "transaction too large" issue with the config lines: This is probably because sugar tries to squeeze as many lines as possible into one tx in src/deploy/config_lines.rs around line 83. The best solution would probably be to get the size of the compute unit price instruction and reduce the MAX_TRANSACTION_BYTES value by it. The faster solution would be to just reduce it by 44 bytes. (According to my quick check using umi this should be the size of the compute unit price ix)

blockchain-bros commented 6 months ago

Hey @blockchain-bros, thank you very much for the PR! Regarding the "transaction too large" issue with the config lines: This is probably because sugar tries to squeeze as many lines as possible into one tx in src/deploy/config_lines.rs around line 83. The best solution would probably be to get the size of the compute unit price instruction and reduce the MAX_TRANSACTION_BYTES value by it. The faster solution would be to just reduce it by 44 bytes. (According to my quick check using umi this should be the size of the compute unit price ix)

Yeah, that would do the trick! I was looking for a way to reduce the TX size but fell short in my attempts. Made this change and tested on devnet with zero issues.

Only tested with 10 assets mind, but I don't think size matters.

MarkSackerberg commented 6 months ago

Hey @blockchain-bros I just gave it a testrun - nice! Exactly as i imagined.

One last minor thing: I think we should remove set_compute_unit_limit. It only has to be used when you want to increase the limit to fit more into one tx, or if you can reduce the default. In our case we do not calculate it manually, but a constant was added. I do not see any need for this. What i would do is remove all those instructions that you added. If you see any need to have them in there please let me know!

    let compute_units = ComputeBudgetInstruction::set_compute_unit_limit(COMPUTE_UNITS);
blockchain-bros commented 6 months ago

Hey @blockchain-bros I just gave it a testrun - nice! Exactly as i imagined.

One last minor thing: I think we should remove set_compute_unit_limit. It only has to be used when you want to increase the limit to fit more into one tx, or if you can reduce the default. In our case we do not calculate it manually, but a constant was added. I do not see any need for this. What i would do is remove all those instructions that you added. If you see any need to have them in there please let me know!

    let compute_units = ComputeBudgetInstruction::set_compute_unit_limit(COMPUTE_UNITS);

Sure, just remove all the compute unit lines. No problem.

Do you want to keep the one that was originally there? https://github.com/metaplex-foundation/sugar/blob/886c6f063b40a0f3504a23711268d11d85d226a8/src/mint/process.rs#L296

Looking good in production run too btw. Doing one now.

MarkSackerberg commented 6 months ago

thank you very much @blockchain-bros !

blockchain-bros commented 6 months ago

thank you very much @blockchain-bros !

My pleasure :)

MarkSackerberg commented 6 months ago

thank you very much @blockchain-bros !

My pleasure :)

Do you have a twitter account by chance that i could tag in the thank you message for your contribution?

blockchain-bros commented 6 months ago

thank you very much @blockchain-bros !

My pleasure :)

Do you have a twitter account by chance that i could tag in the thank you message for your contribution?

Oh sure, that's https://twitter.com/sdrive_app :)