[CAPPL-400] Fix/truncate workflow names #2 #15896
Conversation
justinkaseman
changed the title
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
core/services/workflows/engine.go
Outdated
| // NOTE: Use in logging and metrics can use the longer and human friendly names | ||
| hashTruncateName := workflows.HashTruncateName(cfg.WorkflowName) | ||
| var htNameBytes []byte = hashTruncateName[:] | ||
| hashTruncateNameHex := hex.EncodeToString(htNameBytes) |
There was a problem hiding this comment.
how about we do the hex encoding to ensure UTF8 encoding in the HashTruncateName() so we hexName out of the HashTruncateName() is ready do use.
There was a problem hiding this comment.
This isn't backwards compatible and will affect existing feeds
The name is important and needs to be stable for Aptos/DataFeeds since it's used to authorize the workflow in the consumer contract.
Maybe we can instead inject a function to transform the string name to the hexName? That could then default to what we currently have, but be overridden in the syncer to use our custom logic.
core/services/workflows/engine.go
Outdated
| Workflow sdk.WorkflowSpec | ||
| WorkflowID string | ||
| WorkflowOwner string | ||
| WorkflowName string |
There was a problem hiding this comment.
@justinkaseman Please add comments here detailing what should be used when:
- WorkflowName is the full, human-readable workflow name
- WorkflowNameTransform is responsible for translating the human-readable workflow name to its onchain format (i.e. hex encoded, max 10 bytes)
| @@ -1300,6 +1301,10 @@ func NewEngine(ctx context.Context, cfg Config) (engine *Engine, err error) { | |||
| workflow.hexName = hex.EncodeToString([]byte(cfg.WorkflowName)) | |||
There was a problem hiding this comment.
is it simpler to not add the WorkflowNameTransform field (it's public and could get messed with in theory), not change the syncer and instead just do:
workflow.hexName = pkgworkflows.HashTruncateName(cfg.WorkflowName)
otherwise, I'm no longer a fan of the name and not a fan of something that has to be a fixed length being a public field (in hindsight having a public function be passed was no guarantee of hexName length either)
There was a problem hiding this comment.
That's what I originally tried to do, but as @cedric-cordenier caught here that would cause a breaking change for the existing Keystone feeds
|
Supersedes #15864