refactor(protocol): extract shared MastForestScript type - #3516
Conversation
d998e12 to
ef51882
Compare
90c8b8a to
7ccbbcf
Compare
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me, great observation!
Left comments for potential follow-ups.
| Self { | ||
| mast, | ||
| entrypoint, | ||
| package_debug_info: None, | ||
| } |
There was a problem hiding this comment.
Might not be from this PR, and does not need to be addressed here, but can we refactor this so we have a single place where the type is instantiated (Self { ... })?
I think not following this rule is the number 1 cause of all bugs we've encountered so far. We've had countless instances where some constructor did and some other did not enforce a check. Having a single place for instantiating a struct makes it way easier to reason about. (Recent examples: #3411, #3494). This type is a good example of being hard to reason about, as we have four places where the struct is instantiated, so understanding what the type really enforces is hard.
We have also mostly moved away from panicking constructors, and I would suggest removing it. That would be especially good because we use it in deserialization code, and we should definitely not panic there.
There was a problem hiding this comment.
Fully agree here! It should be relatively easy to refactor such that from_parts is the canonical instantatior returning a Result<Self, MastForestScriptError>. All other methods could just wrap it with their own invariant checks.
In any case, I think these are better left for a follow-up PR.
| pub(crate) fn from_parts_with_package_debug_info( | ||
| package: &Package, | ||
| mast: Arc<MastForest>, | ||
| entrypoint: MastNodeId, | ||
| ) -> Self { |
There was a problem hiding this comment.
This seems like an internal helper and it reads a bit awkward, since the MastForest being passed is the one from Package, so it feels redundant. Consider removing it.
There was a problem hiding this comment.
Agreed. It's only used in TransactionScript::from_package currently, so I think it'd be good to tackle this in tandem with #3516 (comment).
4 participants
Closes #3189
Extracts the
MastForestScripttype andMastForestScriptErrorthat back bothNoteScriptandTransactionScript, movingTransactionScriptinto its owntransaction::scriptmodule. The wrappers now delegate to the shared type instead of duplicating the logic, and resolution errors are unified underMastForestScriptError.