Skip to content
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

multi: move more functionality into taroscript, add TxValidator interface #115

Merged
merged 4 commits into from
Sep 13, 2022

Conversation

jharveyb
Copy link
Collaborator

Spun off from ongoing state machine work.

In this PR, we refactor functions from the VM and Address package to resolve underlying dependency issues and potential cycles. We also introduce the TxValidator interface, to simplify use of the Taro VM from the send state machine.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM, mostly nits and/or clarification questions 🎉

taroscript/send.go Show resolved Hide resolved
taroscript/send_test.go Outdated Show resolved Hide resolved
vm/error.go Outdated Show resolved Hide resolved
vm/vm.go Outdated Show resolved Hide resolved
taroscript/interface.go Outdated Show resolved Hide resolved
tx_validator.go Outdated Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🥦

Changes read well, just one or two minor things re spacing and also the error alias/assignment thing the other reviewer pointed out.

taroscript/send_test.go Outdated Show resolved Hide resolved
@@ -170,6 +172,9 @@ func initSpendScenario(t *testing.T) spendData {
// Generate matching TaroCommitments.
updateScenarioCommitments(t, &state)

// Validator instance needed to call the Taro VM.
state.validator = &taro.ValidatorV0{}
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't happen rn, but I think this would create a circular dep if/when the main package ever imports taroscript. Not something we need to worry about rn tho.

@guggero guggero merged commit ec89edb into main Sep 13, 2022
@guggero guggero deleted the validator-interface branch September 13, 2022 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants