feat: refund submission fee to quorum agreement submitters - #219
JayWhite2357 wants to merge 8 commits into
Conversation
This defines the account id associated with a TableIdentifier. This is a domain key: `sxt/table` that ensures this account id will not collide with any other account ids
a3ddf66 to
7cc508c
Compare
1.70.0Bug Fixes
Features
|
7cc508c to
070d4a0
Compare
| } | ||
|
|
||
| /// Takes a table identifier and returns the deterministic `AccountId` of its treasury. | ||
| pub fn account_id_from_table_id<T: frame_system::Config>( |
There was a problem hiding this comment.
NIT: this function name to me implies that there is some cannonical conversion between table identifiers and accounts, whereas this is more of a specific derivation. I'd prefer something like table_treasury_account
| )) | ||
| .ok() | ||
| } | ||
|
|
There was a problem hiding this comment.
Could use a unit test, specifically one whose test case is generated by the UI Miguel wrote for this
| refund, | ||
| Preservation::Expendable, | ||
| ) { | ||
| Pallet::<T, I>::deposit_event(Event::RefundError { recipient, error }); |
There was a problem hiding this comment.
Is the idea of emitting an event instead of erroring that... this basically means the table is probably underfunded, and it ends up costing indexers to submit to it, but they're incentivized not to since they're not getting payed? And then we never just drop inserts in the event that, say, a system table is underfunded?
| /// The refund is the submission's actual extrinsic fee (computed the same way | ||
| /// `pallet_transaction_payment` does, given `weight`) scaled by the table's configured | ||
| /// submission fee refund percentage (`100` = full refund, `150` = 1.5x, etc), stored under | ||
| /// the `REFUND_PERCENTAGE_DOMAIN` table metadata entry. If no percentage is configured, or | ||
| /// its bytes fail to decode as a `u16`, no refund is made. |
There was a problem hiding this comment.
NIT: This is re-explained on the const docs you've added, it'd be good to have it in just one place and intra-doc-link
| .and_then(|domain| pallet_tables::TableMetadata::<T>::get(&domain, &quorum.table)) | ||
| .and_then(|bytes| u16::decode(&mut bytes.as_slice()).ok()) |
There was a problem hiding this comment.
I think it would make sense to have events for these scenarios instead of nothing. If a table owner really doesn't want to give out refunds, they could set the percentage to 0. I guess maybe the concern is that we'd suddenly have a lot more events for inserts to the existing tables we're indexing to?
There was a problem hiding this comment.
My thought is that if the table refund percentage is not set or invalid, that means "no refunds".
Since refunds are optional and by default off, when there are no refunds, I feel like it would be odd to have a "No refund configured" event.
This doesn't change anything since `finalize_quorum` already only uses it by reference.
…et-indexing's `Config` Needed so the pallet can later draw on a table's treasury balance and compute the real extrinsic fee for a submission when refunding it.
This is the `pallet_tables::TableMetadata` domain key that will store a table's submission fee refund percentage.
In this implementation, only agreements are refunded. Late submissions and dissents are not refunded. That change is left for later.
070d4a0 to
556fb73
Compare
2 participants
Rationale for this change
We wish for data submissions to be refunded from the table treasuries they are submitted to.
What changes are included in this PR?
pallet_transaction_payment::compute_feeusing the real submitted extrinsic's length.Are these changes tested?
Yes.