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

Add support for storage and overhead price #750

Merged
merged 12 commits into from
Aug 8, 2024

Conversation

elmato
Copy link
Contributor

@elmato elmato commented Jul 8, 2024

No description provided.

@elmato elmato requested a review from yarkinwho July 8, 2024 23:59
@elmato elmato changed the title Add support for storage and overhead price [wip] Add support for storage and overhead price Jul 10, 2024
@elmato elmato changed the title [wip] Add support for storage and overhead price Add support for storage and overhead price Jul 29, 2024
};
}
};
VALUE_PROMOTER(uint64_t);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about
typedef uint64_t evm_version_type;
VALUE_PROMOTER(evm_version_type);
to reduce potential ambiguity.


std::optional<pending> pending_version;
uint64_t cached_version=0;
struct gas_prices {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe rename to gas_prices_type to make it consistent with consensus_parameter_data_type

Also, consider change the name "gas_prices" everywhere to avoid confusion with gas_price (no s)

};

using evmtx_type = std::variant<evmtx_v0>;
struct evmtx_v1 : evmtx_base {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we still keep the base price in evmtx_v3 so that it would be possible to implement the option to still use old model with high eos evm version?

@elmato elmato requested a review from yarkinwho August 2, 2024 17:41
Copy link
Contributor

@yarkinwho yarkinwho left a comment

Choose a reason for hiding this comment

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

Seems fine, but two issue:
1 will gas_price and gas_prices cause confusion?
2 when we upgrade from v2 to v3, we stop using gas_price_queue. seems there missing some mechanism to clear the value stored (if any) n that table afterwards?

@elmato
Copy link
Contributor Author

elmato commented Aug 5, 2024

Seems fine, but two issue: 1 will gas_price and gas_prices cause confusion? 2 when we upgrade from v2 to v3, we stop using gas_price_queue. seems there missing some mechanism to clear the value stored (if any) n that table afterwards?

  1. They are similar but the types/functions/actions are very different. We couldn't find a better name for the pair storage_price/overhead_price :/.

  2. We don't allow the change from v2 to v3 if there are pending prices in the queue. Seems that check is missing in the current PR. Will update it

@elmato elmato merged commit df8db94 into main Aug 8, 2024
3 checks passed
@elmato elmato deleted the elmato/add-overhead-storage-price branch August 8, 2024 01:47
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.

2 participants