Introduce priority fee field for transaction and receipt #11228
Conversation
There was a problem hiding this comment.
Looks about right to me.
My main comment is more of a discussion around serialization.
Functionally, I only question the priority policy on deleted account balance refunds, which may or may not be correct already.
And yeah, this is a nasty change... I am glad I only have to review it and didn't have to implement it. :P
But definitely 👍 on doing this change of fields in its own PR, so the meat of the changes can be in more focussed PRs.
| /// Gas refund does not inherit priority from its parent receipt and has no priority associated with it | ||
| /// NOTE: The access key may be replaced by the owner, so the execution can't rely that the | ||
| /// access key is the same and it should use best effort for the refund. | ||
| pub fn new_gas_refund( | ||
| receiver_id: &AccountId, | ||
| refund: Balance, | ||
| signer_public_key: PublicKey, | ||
| priority: ReceiptPriority, | ||
| ) -> Self { | ||
| Receipt { | ||
| predecessor_id: "system".parse().unwrap(), | ||
| receiver_id: receiver_id.clone(), | ||
| receipt_id: CryptoHash::default(), | ||
| match priority { | ||
| ReceiptPriority::Priority(priority) => Receipt::V1(ReceiptV1 { | ||
| predecessor_id: "system".parse().unwrap(), | ||
| receiver_id: receiver_id.clone(), | ||
| receipt_id: CryptoHash::default(), | ||
|
|
||
| receipt: ReceiptEnum::Action(ActionReceipt { | ||
| signer_id: receiver_id.clone(), | ||
| signer_public_key, | ||
| gas_price: 0, | ||
| output_data_receivers: vec![], | ||
| input_data_ids: vec![], | ||
| actions: vec![Action::Transfer(TransferAction { deposit: refund })], | ||
| receipt: ReceiptEnum::Action(ActionReceipt { | ||
| signer_id: receiver_id.clone(), | ||
| signer_public_key, | ||
| gas_price: 0, | ||
| output_data_receivers: vec![], | ||
| input_data_ids: vec![], | ||
| actions: vec![Action::Transfer(TransferAction { deposit: refund })], | ||
| }), | ||
| priority, | ||
| }), |
There was a problem hiding this comment.
I think I agree with gas refunds not inheriting priority, as described in the comment.
But doesn't the code do the opposite? Based on the comment, I would expect that instead of priority as a new parameter to this function, we add a new parameter protocol_version and selects between NoPriority or Priority(0) depending on feature activation.
There was a problem hiding this comment.
Yeah I don't want to introduce a new protocol version in this PR (because it is supposed to be a no-op) so I didn't go with that approach. I can also change this code to only generate V0 for now if it is confusing
| result.new_receipts.push(Receipt::new_balance_refund( | ||
| &delete_account.beneficiary_id, | ||
| account_balance, | ||
| ReceiptPriority::NoPriority, |
There was a problem hiding this comment.
Shouldn't this inherit the priority? Or is that only for balance refunds when execution failed?
There was a problem hiding this comment.
Because no priority generates v0. I want this PR to involve no protocol change so have to do it this way
| /// A receipt type | ||
| pub receipt: ReceiptEnum, | ||
| /// Priority of a receipt | ||
| pub priority: u64, |
There was a problem hiding this comment.
could we also wrap this with a struct such as
pub struct Priority(u64)
to not use u64 directly but the surrounding type?
also is 0 top priority and the larger the number the less priority? can you add comment on this?
There was a problem hiding this comment.
yeah that is a good point. How exactly priority works has not been decided.
There was a problem hiding this comment.
I think I'll address this issue in a following PR as this one is quite large and is difficult to merge into master due to conflicts.
|
@jakmeier @tayfunelmas I plan to merge this PR and iterate on some of the comments in a following one as it is getting quite large and difficult to merge into master. Also, I don't think we need u64 for priority and a u16 is likely sufficient. |
Makes sense. Even just to avoid endless merge conflicts, let's merge this sooner rather than later. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11228 +/- ##
==========================================
- Coverage 70.99% 70.98% -0.01%
==========================================
Files 781 782 +1
Lines 155507 156203 +696
Branches 155507 156203 +696
==========================================
+ Hits 110405 110885 +480
- Misses 40331 40506 +175
- Partials 4771 4812 +41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bowenwang1996
force-pushed
the
transaction-priority-fee
branch
from
d51992f to
791f94a
Compare
A first step towards near/NEPs#541 by introducing a priority field in both transaction and receipt. This is not entirely trivial due to the need to maintain backward compatibility. This PR accomplishes backward compatibility by leveraging the account id serialization and implement manual deserialization for the new transaction and receipt structures. While this PR appears to be quite large, most of the changes are trivial. The core of the changes are the serialization/deserialization of transaction and receipt.
While this change introduces the new versions, they are prohibited from being used in the current protocol until the introduction of the protocol change that leverages priorities.