feat(admin): create admin namespace and add paymaster clear / toggle for reputation #570
Conversation
0xfourzerofour
force-pushed
the
josh/reputation-paymaster-flag
branch
from
da8caa0
to
1d89593
Compare
Codecov ReportAttention:
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
|
0xfourzerofour
force-pushed
the
josh/reputation-paymaster-flag
branch
from
1d89593
to
057cb6c
Compare
0xfourzerofour
force-pushed
the
josh/reputation-paymaster-flag
branch
from
3117623
to
9f92078
Compare
| // Only return an error if tracking is enabled | ||
| if paymaster_metadata.pending_balance.lt(&max_op_cost) && self.tracker_enabled { |
There was a problem hiding this comment.
Something that confused me about the naming conventions here. We're still actually preforming the tracking, correct? We're just not allowing the tracking to cause UOs to be rejected?
There was a problem hiding this comment.
Yeh the naming was something I was not too sure about either. Maybe like only_tracking or blocking_enabled would be more fit for purpose
| @@ -104,7 +104,7 @@ where | |||
| async fn bundler_clear_mempool(&self) -> RpcResult<String> { | |||
| let _ = self | |||
| .pool | |||
| .debug_clear_state(true, false) | |||
| .debug_clear_state(true, true, false) | |||
There was a problem hiding this comment.
Double checking this, we want clear_mempool to clear paymaster tracking? It seems like we should have the clear_mempool bool just wipe out the pending paymaster balances, without actually wiping the whole paymaster tracker.
There was a problem hiding this comment.
Yeh this one I was sort of pre-empting another pr that I am working on that changes how the mempool is split up as I think the pool state should be seperate from paymaster and reputation as they are not directly tied to the pool they are just used to track existing and incoming user ops.
There was a problem hiding this comment.
And also moving the reputation into the same level as the pool so that we do not need that interface
There was a problem hiding this comment.
So we want to leave this as true for clearing the paymaster tracking?
There was a problem hiding this comment.
yeh I think leave it for true if we want to clear the mempool as the paymaster tracking relies on operation id's that are in the mempool
|
Related: #573 |
[Closes] #543 #528
Proposed Changes
current logic
admin_togglePaymasterTracker,admin_toggleReputationTrackerandadmin_clearPaymasterTrackerStateideal logic
While working on this I realized how much I do not like the current reputation manager setup and would like to refactor quickly before this ticket is merged. Currently the strange seperation beween the locked pool paymaster logic and the locked reputation makes things inconsistent.
I started this feature by separating the toggle and clear functions per datastructure like reputation and paymaster however I think it would be better to have 2 admin methods.
admin_clearStatein which there is an enum in the params to choose what state to clear, and if ALL is specified we can clear the pool state. This creatly reduces the amount of boiler plate code needed to get this feature working well.admin_setTrackingin which there is an enum in the params to choose what state to clear, and if ALL is specified we can toggle the pool tracking of each optionpaymasterorreputation. This also reduces the amount of boiler plate code needed.I also would like to move the reputation state within the locked pool state to keep things consistent.