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_button should be a part of FilterAuthority widget

    Current 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.smth will cause query to happen. Instead of that in a method like this create authority_roles_body = self.authority_roles_body and 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 either pattern: 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 WaxAuthority or 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 UpdateAlarmsData and UpdateNodeData. 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 clear and add_suggesion instead of this load_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_suggestions first then add_suggestion)
  • @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 teardown method available on the HiveChainApi because as I see it uses WaxApiCaller which has a base class of AsyncHived. This uses aiohttp communicator which causes the need to call teardown

    I 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_interface and placed under _setup

  • @mzebrak started a discussion:

    should be public_key_to_match here

  • comment in collapsible title fix:

    I think this hardcoded "all" can be improved. Maybe AccountSelectionList should expose smth like is_all_selected?

Edited by Mateusz Żebrak