Follow-up from "Draft: Authority view"
The following discussions from !603 (merged) should be addressed:
-
@mzebrak started a discussion: (+2 comments) I think we should consider having a dialog version of NewKeyAlias
-
@mzebrak started a discussion: (+2 comments) similar as above - all the
account_toggled,clear_filters,request_authority_filter_with_event,request_authority_filter_by_buttonshould be a part of FilterAuthority widgetCurrent structure is an architecture issue and you won't be able to reuse FilterAuthority widget in the modification screen. All widget-related logic should be placed in the widget.
-
@mzebrak started a discussion: This FilterAuthority should have its own .py module - will be reused on the modification screen. move it with its children widgets
-
@mzebrak started a discussion: Looks like we should have some CliveCollapsible that will provide the interface for setting the right-handed text (used now for
threshold) and will have some basic styling -
@mzebrak started a discussion: (+2 comments) Remove "memo_key" from the loop if theres entirely different logic for that
-
@mzebrak started a discussion: Why do you accept a container (dict) if there's only one entry?
-
@mzebrak started a discussion: (+1 comment) If you refer to property that is querying something - each
self.smthwill cause query to happen. Instead of that in a method like this createauthority_roles_body = self.authority_roles_bodyand use that. Look for other places. -
@mzebrak started a discussion: Needs some investigation
-
@mzebrak started a discussion: Instead of nesting so many times, see how it can be done in other validators that can raise many errors.
-
@mzebrak started a discussion: (+6 comments) why different name when can be the same?
-
@mzebrak started a discussion: Why is
pattern: str | list[str]should be eitherpattern: list[str]or*patterns: str. Looks like varargs will fit the best here. Multiple places -
@mzebrak started a discussion: (+1 comment) Looks like we should have some wrapper around
WaxAuthorityor wax interface should be extended. Probably both but I think we'll need a wrapper anyway. -
@mzebrak started a discussion: (+1 comment) -- I think that can be done later --
It looks like we should collect authorities data periodically, as we do with
UpdateAlarmsDataandUpdateNodeData. Then it should be stored in our Account model. It can also be saved on the disk.And this place is good for some wrapper around WaxAccountAuthority
-
@mzebrak started a discussion: This makes no sense for me. It there is already suggester and someone wants entirely new suggestions - why clear old suggester instead of just always creating a new one?
ALso I think maybe better would be to expose
clearandadd_suggesioninstead of thisload_new_suggestions. Its similar to list append/clear so it allows for both:- adding additional suggestions (call
add_suggestion - adding entirely new suggestions (call
clear_suggestionsfirst thenadd_suggestion)
- adding additional suggestions (call
-
@mzebrak started a discussion: Look for other place.
@on(Pressed) def add_private_key(self) -> None: from clive.__private.ui.screens.config.manage_key_aliases.new_key_alias import NewKeyAlias self.app.push_screen(NewKeyAlias(public_key_to_validate=self.public_key), self._key_aliases_changed_callback) -
@mzebrak started a discussion: You can basically use a set so there won't be duplicates. Also add_suggestion removes duplicated.
input_suggestions.insert(collected_entry) input_suggestions.update(collect_all_aliases_from_profile()) -
@mzebrak started a discussion: widgets: list[Widget] -
@mzebrak started a discussion: I wonder if this should refresh automatically when authority changes in the blockchain
-
@mzebrak started a discussion: should be a separate private method like
_collect_authorities -
@mzebrak started a discussion: yield in compose and query instead
-
@mzebrak started a discussion: Do it in a proper way instead of
multiple_patterns if "multiple_patterns" in locals() else filter_pattern -
@mzebrak started a discussion: This needs refactor. Looks like some of the logic should be placed in AuthorityInput
-
@mzebrak started a discussion: This MR needs more attention (refactor) on the implementation details.
An example of such a refactor can be this place.
@on(SelectionList.SelectionToggled) def _handle_selection(self, event: AccountSelectionList.SelectionToggled[str]) -> None: """ Handle selection toggling - manage the "all" option behavior and its interaction with other options. When "all" is selected, all other options are selected. When "all" is deselected, all other options are deselected. When any other option is selected while "all" is selected, "all" gets deselected. """ if event.selection.value == "all": if "all" in self.selected: self.select_all() else: self.deselect_all() elif "all" in self.selected: self.deselect("all") -
@mzebrak started a discussion: (+1 comment) I'm surprised there is no
teardownmethod available on theHiveChainApibecause as I see it usesWaxApiCallerwhich has a base class ofAsyncHived. This uses aiohttp communicator which causes the need to callteardownI think I mentioned that some time ago to @jziebinski, but IDK if that is available in some newer version of wax or if it hasn't been added yet
-
@mzebrak started a discussion: (+2 comments) Key or account? But you can't create PublicKey from account.
-
@mzebrak started a discussion: Looks like should be named
_setup_wax_interfaceand placed under_setup -
@mzebrak started a discussion: should be
public_key_to_matchhere -
comment in collapsible title fix: I think this hardcoded
"all"can be improved. MaybeAccountSelectionListshould expose smth likeis_all_selected?