New QueryRunner API

[bidzyyys]

No description provided.

[matthias-wright]

nice, looks good overall! I don't really like the macros do_keys, do_query, and query. I think they make the code less readable and we are not gaining much by having them there. they way it was before, where we explicitly called self.inner.run with the context seemed fine to me.

[bidzyyys]

nice, looks good overall! I don't really like the macros do_keys, do_query, and query. I think they make the code less readable and we are not gaining much by having them there. they way it was before, where we explicitly called self.inner.run with the context seemed fine to me.

@matthias-wright changes applied!

[matthias-wright]

nice, looks good overall! I don't really like the macros do_keys, do_query, and query. I think they make the code less readable and we are not gaining much by having them there. they way it was before, where we explicitly called self.inner.run with the context seemed fine to me.

@matthias-wright changes applied!

Thanks 🙏

[kckeiks]

LGTM

[daltoncoder]

Looks good. A couple suggestions to clean up the SyncQueryRunner interface even more. Id like to to keep the interface limited to direct state table access, and not for specific data within them. To do this we could add another function that takes a closure for the tables we need to iter over(just the NodeInfo table)
We would add this this to the interface:

    pub fn get_node_table_iter(&self) -> KeyIterator<NodeIndex> {
        self.inner.run(|ctx| self.node_table.get(&ctx).keys())
    }    pub fn get_node_table_iter(&self) -> KeyIterator<NodeIndex> {
        self.inner.run(|ctx| self.node_table.get(&ctx).keys())
    }

Then the following functions could be removed from the interface:
get_chain_id
get_committee_members
get_current_epoch
get_epoch_info
get_current_latencies
get_genesis_committee
get_last_block
get_node_registry
get_staking_amount
is_valid_node

We can add these functions to a util file, core/types/src/state_queries.rs, that take a SyncQueryRunner as a param and get this data from the provided functions. Things that are already using these functions can import it from there instead. Its a little bit of refactor but I think it stops the pattern we have been doing of adding to the interface to get new specific data from state.

Edit: Another idea is to make a QueryRunnerExt trait in types that autoimplements those functions we are removing. Then everywhere else we can just bring this trait into scope.

[daltoncoder]

Suggested change

	fn get_account_info<F: Clone>(
	#[inline]
	fn get_account_info<F: Clone>(

Lets mark all functions that take impl FnOnce as #[inline] in the implementation to avoid the runtime lookup

[bidzyyys]

good catch

[daltoncoder]

Suggested change

	fn get_node_info<F: Clone>(
	fn get_node_info<T>(

None of these returns should need to be Clone,
Minor nit: Across all these functions lets rename the generic to T since its the return type of the closure and that is the typical naming convention. F makes me think it is the closure itself

[bidzyyys]

I can remove Clone requirement.
Regarding F -> T we cannot do that. I posted a message some time ago in our internal Discord channel that with #[infusion::blank] trait we cannot use T parameter (probably it injects it somewhere). @daltoncoder if you wish I can change F to sth else.

[kckeiks]

I think he was objecting to F because it's commonly used for function-like bounds so I think it's fine to change it to anything else.

[daltoncoder]

Ah yeah i missed that message about infusion injecting T already. Let me work on getting a fix for that. In the meantime we can keep this F or whatever we want

[bidzyyys]

Changing to V then!

[daltoncoder]

Nit: Since this table is mapping ClientKey->EthAddress(AccountAddress) lets rename this function in interface to client_key_to_account_key

[ozwaldorf]

+1 for the trait extension pattern for the auto impl utilities, similar to AsyncRead and AsyncReadExt

[bidzyyys]

Thanks @daltoncoder & @ozwaldorf for the input, I like the idea of trait extension pattern!

[ozwaldorf]

lgtm