fix: identity registration events to use keyper-set-index#691
fix: identity registration events to use keyper-set-index#691blockchainluffy wants to merge 2 commits intomainfrom
Conversation
| continue | ||
| } | ||
| eonStruct, err := coreKeyperDB.GetEon(ctx, eon) | ||
| eonStruct, decryptable, err := kpr.resolveDecryptableEon(ctx, coreKeyperDB, eon) |
There was a problem hiding this comment.
signature is
func (kpr *Keyper) resolveDecryptableEon(
ctx context.Context,
coreKeyperDB *corekeyperdatabase.Queries,
keyperConfigIndex int64
) (corekeyperdatabase.Eon, bool, error) {…}In other words: here we implicitly cast between keyperConfigIndex and eon.
There was a problem hiding this comment.
This is honestly puzzling to me, because eon is a value contained in FiredTrigger of service db and should not be a "keyperConfigIndex" kind? Or should not need resolution into a corekeyperdatabase.Eon?
Could it be that the triple return is actually superfluous, because the call does not "resolve" the eon, but only verifies "decryptability", given the current keyper and active keyper set?
There was a problem hiding this comment.
aha, we "resolve" eon properties such as ActivationBlockNumber that we need later on. So I guess we're resolving a type.
IOW: if we want to avoid the odd cast, we might want to consider changing the argument name to eonNumber, because the value we're handling is not strictly a keyperSetIndexkeyperConfigIndex but rather the numeric for a specific eon, right?
There was a problem hiding this comment.
I don’t think renaming the parameter to eonNumber would be correct here. This PR is changing the semantics of the even eon field. The incoming value is no longer being treated as a real core eon number; it is being treated as the submitted keyper-set/config index from the registry event, and resolveDecryptableEon() then resolves the latest eon record for that set. So the oddity is not the function signature, it’s that the service DB field is still named Eon even though this PR changes its semantics. If we want to reduce confusion, the cleaner follow-up is to rename the internal shutter-service representation at the ingestion boundary (e.g. keyperConfigIndex / submittedKeyperConfigIndex) and keep “eon” only for the resolved corekeyperdatabase.Eon. We should do the same on the API side as well: if the API is submitting a keyper-set/config index, we should name it that way there too, rather than continuing to call it eon and relying on implicit reinterpretation downstream.
| // don't trigger if we're not part of the keyper set | ||
| _, isKeyper, err := coreKeyperDB.GetKeyperIndex(ctx, keyperConfigIndex, kpr.config.GetAddress()) | ||
| if err != nil { | ||
| if err == pgx.ErrNoRows { |
There was a problem hiding this comment.
This check won’t fire as intended as GetKeyperIndex() wraps the underlying GetBatchConfig() error, so when the batch config is missing the returned error is not == pgx.ErrNoRows. We should use errors.Is(err,pgx.ErrNoRows) here.
3 participants
closes #692