Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add call flow spec for validation and execution #166
feat: add call flow spec for validation and execution #166
Changes from all commits
a28bea7
97e67a7
ca981c8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the second sentence necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use
pre validation hook(s)
for allvalidation hook(s)
references to be precise and consistent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if that is not standardized, then the modules may be written to expect the data in different locations, which would result in modules that are compatible with some 6900-compliant accounts, but not all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, but I think the fact that there are not
post validation hook(s)
means that the "pre" wording doesn't differentiate it any further. It gives some insight into when to expect it to be run, but that's not really necessary for this section of the spec.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely get what you mean. However, I think there are benefits to have
pre
as it makes it a bit easier to understand how it works for new comers to (4337 and 6900) who does not know a ton how exactly validation works yet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards just calling them validation hooks (adding "pre" might signal that there exists a "post", similar to execution hooks). But I see that we do reference "pre validation hooks" in the prior paragraph. We should be consistent everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point on signaling "post". Happy to go with "validation hooks" with consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, in other words, means that the result of UO validation should always be the longest possible time bound that satisfies all time bounds form hooks , right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Technically I think it is accurate to describe it as the "longest time bound that satisfies hooks", but intuitively, the operation of intersecting them is to take the min. E.g. if you have ascending timestamps$t_1$ , $t_2$ , $t_3$ , $t_4$ , and hook 1 returns the range $(t_1, t_3)$ , and the validation function returns $(t_2, t_4)$ , the result of validation should be the range $(t_2, t_3)$ .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's been decided to just use "validation hooks" as the consistent term yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should state the risk where if the
isPublic
function changes states, omitting validation can be dangerous.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need that extra detail? I think that is obvious from the spec stating that validation can be skipped if the function is marked public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code comment on
isPublic
isWhether or not the function needs runtime validation, or can be called by anyone.
.Why didn't we name it
skipRuntimeValidation
flag?My concern of the name
isPublic
is that it does not indicate that it can potentially be state changing. We only skip runtime by using the oldALWAYS_ALLOW
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does seem like a good idea, makes it more literal. Maybe less obvious to connect it to view functions, but it would match the naming of other flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't have to be view functions is the whole point (to allow state changing though I am not sure what the use cases are, maybe exec timelock tx after trigger time or sth), right?
Otherwise, it can just be
isPublic
orisPublicView
orisView
, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Let's update that in a different PR, because it also requires code changes.