Add NetworkId into BlockInfo
#265
Conversation
lowhung
changed the base branch from
main
to
lowhung/163-replace-reward-account-with-stake-address
Resolves issue #163 This adds `NetworkId` into the `BlockInfo` struct, and introduces this identifier into the account state, drep state, epochs state and rest_blockfrost as these all rely on querying the `StakeAddressMap`.
lowhung
force-pushed
the
lowhung/163-replace-reward-account-with-stake-address-block-info-network-id
branch
from
1bfa43a to
fe8c1c9
Compare
sandtreader
left a comment
sandtreader
left a comment
There was a problem hiding this comment.
Main issue is not to store networkId in state - there's no need, and no sensible place to capture it (first message? Any message?). It should always be available in BlockInfo except for things that create blocks where it's configured (as you have done).
To avoid having to pass it around I've suggested changing parameters to be full StakeAddresses, so the caller (which is usually a message handler) can do the conversion. Even better, the messages should have StakeAddresses instead of StakeCredentials (which may fall away altogether), so it's things like TxUnpacker which do the conversion based on the NetworkID they get in their BlockInfo.
If we apply the same change to the REST query responses I think we can avoid having to configure network in RESTBlockfrost too.
modules/accounts_state/src/state.rs
Outdated
| /// List of SPOs (by operator ID) retiring in the current epoch | ||
| retiring_spos: Vec<KeyHash>, | ||
|
|
||
| /// Network ID |
There was a problem hiding this comment.
We don't need to store this (and it there's no obvious place to capture it anyway) - get it from BlockInfo in the message handlers and pass it down to things that need it, or convert to StakeAddress before calling them.
modules/drep_state/src/state.rs
Outdated
| pub struct State { | ||
| pub config: DRepStorageConfig, | ||
| pub dreps: HashMap<DRepCredential, DRepRecord>, | ||
| pub network_id: NetworkId, |
There was a problem hiding this comment.
No need to store, as above
modules/drep_state/src/state.rs
Outdated
| let stake_keys: Vec<KeyHash> = delegators.iter().map(|(sc, _)| sc.get_hash()).collect(); | ||
| // TODO: USE NETWORK ID | ||
| let stake_addresses: Vec<StakeAddress> = delegators.iter().map(|(k, _) | k.to_stake_address(None) ).collect(); | ||
| let stake_addresses: Vec<StakeAddress> = delegators.iter().map(|(k, _) | k.to_stake_address(self.network_id.clone().into()) ).collect(); |
There was a problem hiding this comment.
I'd make delegators a Vec<(&StakeAddress, &DRepChoice)> which will push the need for networkId out to the caller and make it easier to replace StakeCredential entirely if required.
| .get_string("network-id") | ||
| .unwrap_or(DEFAULT_NETWORK_ID.to_string()), | ||
| ); | ||
|
|
| pub parameters_query_topic: String, | ||
| pub external_api_timeout: u64, | ||
| pub offchain_token_registry_url: String, | ||
| pub network_id: NetworkId, |
There was a problem hiding this comment.
Hmm, shame we have to configure this here... Can we change the queries so the data comes back as StakeAddresses so we don't need to? With the pool owners I think we talked about doing that anyway...
modules/spo_state/src/state.rs
Outdated
|
|
||
| epoch: u64, | ||
|
|
||
| network_id: NetworkId, |
There was a problem hiding this comment.
No need to store, as above
modules/spo_state/src/state.rs
Outdated
| }; | ||
| let mut stake_addresses = stake_addresses.lock().unwrap(); | ||
| stake_addresses.register_stake_address(&credential.to_stake_address(None)); | ||
| stake_addresses |
There was a problem hiding this comment.
As above, change parameter to StakeAddress and make the caller do the conversion so we don't need the networkID here.
processes/omnibus/omnibus.toml
Outdated
| [module.block-unpacker] | ||
|
|
||
| [module.rest-blockfrost] | ||
| network = "mainnet" |
There was a problem hiding this comment.
Hopefully we can lose this one. The others make perfect sense.
…ddress` handling in tx_unpacker.rs - Removed `NetworkId` from structures and functions across modules. - Replaced `KeyHash` and `Credential` usage with `StakeAddress` for consistency.
lowhung
force-pushed
the
lowhung/163-replace-reward-account-with-stake-address-block-info-network-id
branch
from
b876970 to
b166d93
Compare
…rgets and simplify handling
lowhung
force-pushed
the
lowhung/163-replace-reward-account-with-stake-address-block-info-network-id
branch
from
b166d93 to
de08f0a
Compare
lowhung
merged commit 2faea48
into
lowhung/163-replace-reward-account-with-stake-address
SupernaviX
deleted the
lowhung/163-replace-reward-account-with-stake-address-block-info-network-id
branch
Description
Resolves issue #163
This adds
NetworkIdinto theBlockInfostruct via the Mithril and Upstream fetchers, and introduces the identifier into the account state, drep state, epochs state and rest_blockfrost as these all rely on querying theStakeAddressMapwhich has had it's API updated to requireStakeAddressstructs as its key.