feat(ntx-builder): bundle and execute sponsorship and feature notes - #2514
Conversation
b0fae0b to
fd9bf91
Compare
SantiagoPittella
left a comment
There was a problem hiding this comment.
Overall looks good, thanks for the improvements. We need to modify the changelog entry in the PR description to remove the "--require-sponsorship" flag.
Left some comments, mostly nits and could be addressed in a followup pR
| }; | ||
| // Prefer the largest fee contributions both when applying the cap and when filtering | ||
| // later walks down successively smaller sponsorship prefixes. | ||
| group.sort_sponsorships_by_amount(); |
There was a problem hiding this comment.
The thing with this sort is that someone could create a faucet and create 3 sponsorship notes for the feature note using an amount that will always "win" the 3 slots, and then fail at execution since we aren't checking the asset ID, right?
There was a problem hiding this comment.
Yeah, that's true. We can read the storage slot and use that to retain "valid" sponsorship notes, or we can remove the ordering altogether and make it random or arbitrary in some other way. I went with the first approach but perhaps for simplicity we should do the latter approach?
There was a problem hiding this comment.
I prefer the first approach 👍🏼
There was a problem hiding this comment.
At the moment we should only accept fees in the native asset - everything else is invalid/unaccepted atm.
There was a problem hiding this comment.
Do you mean the network's native token? Or native as in native to the network faucet? Because I believe nothing enforces anything related to this at the moment
There was a problem hiding this comment.
I might be misunderstanding, but I don't think we need to support multiple token types - we can't even compare/sort them by value yet.
There was a problem hiding this comment.
The protocol forces the sponsorship asset to match the network account configures in its fee policy, but I think that it does not forces a match with the native token
There was a problem hiding this comment.
I might be misunderstanding, but I don't think we need to support multiple token types - we can't even compare/sort them by value yet.
I think nothing forces network account developers to be paid in a specific token (like the native one). AFAICT, they can decide to charge fees from a random asset ID. Obviously these are different than the network fees, which are currently only payable in the native token. Whether this is a problem or not, I'm not sure, but with my comment I mostly wanted to make sure assumptions are aligned.
There was a problem hiding this comment.
Right you are; I was confused
There was a problem hiding this comment.
I ended up re-adding the sort with the filter of expected assets.
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Approving though I'm unsure if all the complexity in searching/grouping is required i.e. could we simplify by taking only the single most valuable sponsor note per feature note and call it a day.
| /// without every selected sponsorship when its required fee is otherwise covered. A group with no | ||
| /// sponsorships is a plain network note. | ||
| #[derive(Clone, Debug)] | ||
| pub struct NoteGroup { |
There was a problem hiding this comment.
Perhaps
| pub struct NoteGroup { | |
| pub struct SponsoredFeatureNote { |
There was a problem hiding this comment.
I suggested that in the original PR as well. I think it works except it implies the note is always sponsored which is not really the case (0 fees is possible and no note is required). I might still change it
There was a problem hiding this comment.
Ended up doing the rename
9e077f0 to
c06ab01
Compare
|
@Mirko-von-Leipzig I think we are ready to merge the stack |
Summary
Part 2 for #2327, and alternative take on #2499
NoteConsumptionCheckerbundle-aware protocol#3710 for more information)GroupIndexand the re-pairing logic. The reasoning here is that if the checker returns succesful notes, they would have been sponsored correctly and so there is never need of re-pairingrequire_sponsorshipCLI/config flag.randdependency.Changelog