Skip to content

Resolve workflow owner by deployment registry#390

Merged
timothyF95 merged 6 commits into
mainfrom
adjust-workflow-owner-logic
Apr 21, 2026
Merged

Resolve workflow owner by deployment registry#390
timothyF95 merged 6 commits into
mainfrom
adjust-workflow-owner-logic

Conversation

@timothyF95
Copy link
Copy Markdown
Contributor

When user-workflow.deployment-registry is set, workflow owner is no longer resolved during the first settings load (which required a local key even for off-chain registries). Owner is filled after the registry is resolved: off-chain uses the session-derived owner with type ORG_DERIVED; on-chain still uses GetWorkflowOwner. Derived owner is normalized once when credentials are validated (FormatWorkflowOwnerAddress). Per-command 0x prefix handling in workflow handlers is removed. Tests and workflow fixture YAMLs added/updated.

Comment thread internal/ethkeys/keys.go
Comment on lines +214 to +215
workflow.UserWorkflowSettings.WorkflowOwnerAddress = ownerAddr
workflow.UserWorkflowSettings.WorkflowOwnerType = ownerType
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered not making this part of settings at all? What if we added a dynamic runtimeContext.ResolveOwner(<params like registry and whatever else we might need>)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is closer to to what we will need as a final solution. I think we are going to have to break the settings down into smaller components and load on request. Will share details offline

ownerAddress, ownerType, err := GetWorkflowOwner(v)
if err != nil {
return WorkflowSettings{}, err
if deploymentRegistry == "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can deploymentRegistry still be empty here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, to maintain backwards compatibility, we must account for an empty deploymentRegistry else this would be a breaking change to current projects. If it is not set, getSetting returns an empty string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I thought we load registry before making this call?

Comment thread internal/settings/workflow_settings.go
Comment on lines +193 to +198
if workflow.UserWorkflowSettings.DeploymentRegistry == "" {
return nil
}
if resolved == nil {
return fmt.Errorf("resolved registry is required to finalize workflow owner")
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check for both?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short answer is yes. If DeploymentRegistry is "" then we should early return as this step is not needed. If resolved is nil then settings are in a broken state and cannot proceed.

@timothyF95 timothyF95 marked this pull request as ready for review April 21, 2026 08:10
@timothyF95 timothyF95 requested a review from a team as a code owner April 21, 2026 08:10
@timothyF95 timothyF95 added this pull request to the merge queue Apr 21, 2026
Merged via the queue into main with commit a2b2e66 Apr 21, 2026
22 checks passed
@timothyF95 timothyF95 deleted the adjust-workflow-owner-logic branch April 21, 2026 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants