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

auction: add GasCost impls for actions #4223

Merged
merged 8 commits into from
Apr 17, 2024
Merged

auction: add GasCost impls for actions #4223

merged 8 commits into from
Apr 17, 2024

Conversation

TalDerei
Copy link
Collaborator

@TalDerei TalDerei commented Apr 16, 2024

Describe your changes

Add GasCost vectors for DA actions, addAction / ActionPlan skeletons, and make gas fee costs consistent with protobuf encodings (#3940)

Issue ticket number and link

References #4212

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

@TalDerei TalDerei self-assigned this Apr 16, 2024
@TalDerei TalDerei marked this pull request as ready for review April 17, 2024 14:04
@TalDerei
Copy link
Collaborator Author

@erwanor @zbuc should we make the DA fees be counted towards the block_space instead of the execution?

@TalDerei TalDerei requested review from hdevalence, zbuc and erwanor April 17, 2024 14:06
@zbuc
Copy link
Contributor

zbuc commented Apr 17, 2024

@erwanor @zbuc should we make the DA fees be counted towards the block_space instead of the execution?

Do you mean the part multiplied by the step count? I think there is both a per-step block space and per-step execution time cost that should be accounted for. Doesn't each step incur a block space cost of storing liquidity positions?

@TalDerei
Copy link
Collaborator Author

TalDerei commented Apr 17, 2024

@erwanor @zbuc should we make the DA fees be counted towards the block_space instead of the execution?

Do you mean the part multiplied by the step count? I think there is both a per-step block space and per-step execution time cost that should be accounted for. Doesn't each step incur a block space cost of storing liquidity positions?

yeah that's true, would the block_space therefore be something like proto encoding cost of action + (dutch_action_schedule.description.step_count * 8 bytes / step)?

@zbuc
Copy link
Contributor

zbuc commented Apr 17, 2024

proto encoding cost of action + (dutch_action_schedule.description.step_count * 8 bytes / step)

I am operating under the assumption (that I am not 100% sure of) that every step, a LP will be closed (and withdrawn?) and a new one opened.

So I think it should be something like proto encoding cost of action + dutch_action_schedule.description.step_count * (PositionOpen block space cost + PositionClose block space cost + PositionWithdraw block space cost)

@hdevalence
Copy link
Member

I don't think I follow — how does the chain opening and closing positions on its own add data to a future block?

@zbuc
Copy link
Contributor

zbuc commented Apr 17, 2024

I don't think I follow — how does the chain opening and closing positions on its own add data to a future block?

I suppose it's not adding new Actions to the block and instead using only additional storage space in the JMT for the position data, and we don't account for JMT space usage in gas pricing.

@erwanor erwanor changed the title fees: DA auction: add GasCost impls for actions Apr 17, 2024
Copy link
Member

@erwanor erwanor left a comment

Choose a reason for hiding this comment

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

Requesting changes, but good to merge as soon as we have addressed the comments + green CI. I would be happy to pair on a second pass that makes the dependency between DA actions and the execution costs of position actions a little better (rather than having reminders in comments). But it seems good enough for now. Thanks for taking this on!

crates/core/transaction/src/gas.rs Outdated Show resolved Hide resolved
crates/core/transaction/src/gas.rs Outdated Show resolved Hide resolved
crates/core/transaction/src/gas.rs Show resolved Hide resolved
@erwanor erwanor merged commit 6c8d3e7 into main Apr 17, 2024
8 checks passed
@erwanor erwanor deleted the auction-fees branch April 17, 2024 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants