feat: migrate transaction scripts to Library#3173
Conversation
`@transaction_script` attribute
TomasArrachea
force-pushed
the
tomasarrachea-tx-script-attribute
branch
from
a63c913 to
af44cea
Compare
| // TODO: we can remove this `Program` based constructor once the compiler integrates the | ||
| // `@transaction_script` attribute (https://github.com/0xMiden/compiler/issues/1190). | ||
| pub fn new(code: Program) -> Self { | ||
| Self::from_parts(code.mast_forest().clone(), code.entrypoint()) | ||
| } |
There was a problem hiding this comment.
Let's do this in a follow up right after this PR. And we should do the same for note scripts.
cc @greenhat for visibility.
| @transaction_script | ||
| pub proc main |
There was a problem hiding this comment.
Not for this PR, but we should move this out into a package (similar to how we do with notes).
mmagician
left a comment
mmagician
left a comment
There was a problem hiding this comment.
LGTM modulo some refactors (but they could be done as a follow-up if we agree they are desirable)
There was a problem hiding this comment.
At this point NoteScript and TransactionScript share the majority of the struct's implementation code, with the only difference being TRANSACTION_SCRIPT_ATTRIBUTE vs. NOTE_SCRIPT_ATTRIBUTE. We might want to pull this functionality into something more re-usable.
There was a problem hiding this comment.
I think this would make sense. Created #3189 for it, so we avoid growing this PR.
| /// Returns a new [TransactionScript] instantiated from the provided library. | ||
| /// | ||
| /// The library must contain exactly one procedure with the `@transaction_script` attribute, | ||
| /// which will be used as the entrypoint. | ||
| /// | ||
| /// # Errors | ||
| /// Returns an error if: | ||
| /// - The library does not contain a procedure with the `@transaction_script` attribute. | ||
| /// - The library contains multiple procedures with the `@transaction_script` attribute. | ||
| pub fn from_library(library: &Library) -> Result<Self, TransactionScriptError> { |
There was a problem hiding this comment.
To align a bit better with NoteScript, it would make sense to pull out the struct TransactionScript into its own file.
(see also my other comment about NoteScript and TransactionScript code de-dup, as the decision there would affect this refactor, too)
`NoteScript::from_library`
TomasArrachea
force-pushed
the
tomasarrachea-tx-script-attribute
branch
from
0c703c6 to
17e21bc
Compare
|
Addressed all the comments. Still, we should wait for #3146 to get merged and then rebase. |
Done! Main change point on the merge was the code builder , where the |
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good, thanks for the updates!
Resolves conflicts from the transaction-script-to-Library migration (#3173): the three conflicted test scripts adopt the new `@transaction_script pub proc main` form but keep our account-context routing (`call.mock_account::*` for the gated account reads, `call.create_note` for note creation) since a transaction script is still not an account-procedure context. Full suite green: 1311 passed, 0 failed.
4 participants
Closes #3093
This PR migrates transaction scripts to the library form and adds a
@transaction_scriptattribute to mark the entrypoint procedure.