Enhancement: `goal` Error UX Improvement by tzaffi · Pull Request #4951 · algorand/go-algorand

tzaffi · 2022-12-28T14:54:48Z

Summary

Changing assembler functionality so that data/transactions/logic/assembler.go::assemble() errors when receiving empty input.

For example, here's what occurs when the clear program is empty (but approval is fine):

❯ goal app create --creator $ADDR_CREATOR \
                --approval-prog "$TEAL_APPROVAL_PROG" \
                --clear-prog "$TEAL_CLEAR_PROG" \
                --global-byteslices $GLOBAL_BYTESLICES \
                --global-ints $GLOBAL_INTS \
                --local-byteslices $LOCAL_BYTESLICES \
                --local-ints $LOCAL_INTS -d /Users/zeph/networks/niftynetwork/Primary
clear_state_program.teal: 1 error 0: Cannot assemble empty program text
clear_state_program.teal: 0: Cannot assemble empty program text

Question for Consideration

Is this the right approach? Is there a reason to allow empty programs to be assembled?

Related, to this. Consider the following empty'ish program:

#pragma version 7

Currently (and this PR doesn't change the behavior) we get the following error when attempting to run goal app create:

Couldn't broadcast tx with algod: HTTP 400 Bad Request: TransactionPool.Remember: transaction VTYB432J3J6XBZX2FPU5J3KWMN6OTEM5PRJLXIEY5ATAREZI66BA: logic eval error: stack len is 0 instead of 1. Details: pc=1, opcodes=

So we're perfectly happy to start the evaluator and immediately stop it, then declare a rejection. The PR here doesn't adhere to this pattern.

Even if the answer to the above is "yes, it is good to allow empty programs to be assembled because ..." there is still the counterargument that we want to fail as fast as possible and not create extra work for the node to evaluate guaranteed-to-fail transactions.

Issues

Confusing error message when approval/clear programs are empty #2585

Test Plan

CI
Downstream SDK's. For example, tests pass in the python SDK

codecov · 2022-12-28T18:56:27Z

Codecov Report

Merging #4951 (5e4f0ea) into master (5d38a15) will increase coverage by 0.02%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #4951      +/-   ##
==========================================
+ Coverage   53.62%   53.65%   +0.02%     
==========================================
  Files         432      432              
  Lines       54060    54068       +8     
==========================================
+ Hits        28991    29011      +20     
+ Misses      22824    22810      -14     
- Partials     2245     2247       +2

Impacted Files	Coverage Δ
cmd/goal/clerk.go	`8.77% <0.00%> (ø)`
cmd/goal/multisig.go	`10.00% <0.00%> (ø)`
daemon/algod/api/server/v2/handlers.go	`0.82% <0.00%> (ø)`
data/transactions/logic/assembler.go	`87.25% <100.00%> (+0.74%)`	⬆️
util/rateLimit.go	`81.37% <0.00%> (-0.99%)`	⬇️
cmd/tealdbg/debugger.go	`72.69% <0.00%> (-0.81%)`	⬇️
ledger/acctupdates.go	`68.99% <0.00%> (-0.25%)`	⬇️
network/wsNetwork.go	`64.74% <0.00%> (-0.18%)`	⬇️
catchup/service.go	`69.80% <0.00%> (+1.20%)`	⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

algochoi

Looking good, I agree that returning a clearer error for an empty program seems better from a UX perspective 👍

algorandskiy · 2022-12-29T17:48:53Z

 // assemble reads text from an input and accumulates the program
 func (ops *OpStream) assemble(text string) error {
+	if strings.TrimSpace(text) == "" {
+		ops.errorf("Cannot assemble empty program text")


return here?

I didn't want to return right away because I want to let the assembler do it's thing and report number of errors. I.e., if I return right away instead of getting the following message:

clear_state_program.teal: 0: Cannot assemble empty program text clear_state_program.teal: 1 error

I would get the following:

clear_state_program.teal: 0: Cannot assemble empty program text clear_state_program.teal: 0: Cannot assemble empty program text

It should be ok to:
return ops.errorf("Cannot assemble empty program text")
since that's how the Version check works a couple lines lower.
return ops.errorf("Can not assemble version %d", ops.Version)
That must show the same problem in the context you're looking at.

Perhaps we can change ops.ReportProblems to only report the underlying errors if there's more than 1? Rename to ops.MultpleErrors?

This worked with the following error goal messages:

clear_state_program.teal: 1 error clear_state_program.teal: 0: Cannot assemble empty program text

After modifying as above, test/scripts/e2e_subs/e2e-teal.sh started failing as the original error wasn't reported for goal clerk compile a logic sig (bad compiler version). So I modified ops.MultipleErrors() to also report the original unique error along with the count in the same line (only in the case of exactly 1 error and no warnings). Now the error looks like:

clear_state_program.teal: 1 error: 0: Cannot assemble empty program text clear_state_program.teal: 0: Cannot assemble empty program text

… 1 error to _not_ report the actual errors, but "1 error" instead

tzaffi · 2023-01-02T01:32:06Z

 }

-func TestMultipleErrors(t *testing.T) {
+func TestSeveralErrors(t *testing.T) {


Renaming as the natural name of a new unit test for renamed function func(ops *OpStream) MultipleErrors is this one.

…rrors

algorandskiy · 2023-01-11T17:36:01Z

+	if strings.TrimSpace(text) == "" {
+		return ops.errorf("Cannot assemble empty program text")
+	}


Suggested change

if strings.TrimSpace(text) == "" {

return ops.errorf("Cannot assemble empty program text")

}

// version check

...

trimmed := strings.TrimSpace(text)

if trimmed == "" {

return ops.errorf("Cannot assemble empty program text")

}

fin := strings.NewReader(trimmed)

Implemented suggestion in 5f7de10

I am reverting this change. Though I agree that passing on trimmed seems sensible, it is breaking tests, and the asserted behavior is there for a reason. For example, when providing this TEAL input:

int 1 #pragma version 2

This test case is failing:

This is because it is expecting the error:

{l: 3, s: "#pragma version is only allowed before instructions"}

but now, since we removed the empty line, it becomes

{l: 2, s: "#pragma version is only allowed before instructions"}

From a user's perspective, I believe the original error is more correct.

tzaffi · 2023-01-11T18:17:20Z

Update: The recent changes are causing test failures. I will investigate further.

bbroder-algo · 2023-01-12T23:33:56Z

+	emptyExpect := Expect{0, "Cannot assemble empty program text"}
+	emptyPrograms := []string{
+		"",
+		"     ",


What about "label:" or " //"

The goal of the PR was to address the original concern (see #2585 ) which is when someone basically accidentally puts in a blank program for assembly. I expanded the definition to include any program that consists of all whitespace. We can expand even further, but it does seem to me to be a can of worms that I am inclined to push back against... However, I'm more than happy to test this out experimentally in our pair programming project.

…rrors

Changed assembler functionality so that logic.assemble errors when receiving empty input

enforce "Cannot assemble empty program text"

89f980a

tzaffi changed the title ~~Improveme Some goal Error UX~~ Improve Some goal Error UX Dec 28, 2022

tzaffi added duplicate This issue or pull request already exists Enhancement labels Dec 28, 2022

tzaffi changed the title ~~Improve Some goal Error UX~~ Enhancement: Improve Some goal Error UX Dec 28, 2022

tzaffi changed the title ~~Enhancement: Improve Some goal Error UX~~ Enhancement: goal Error UX Improvements Dec 28, 2022

Zeph Grunschlag added 3 commits December 28, 2022 09:37

unit test TestAssembleEmpty

59244bf

iterate with uint64 type

d57acc2

TestTealCompile - nil program should NOT work

0e430cc

tzaffi changed the title ~~Enhancement: goal Error UX Improvements~~ Enhancement: goal Error UX Improvement Dec 28, 2022

tzaffi commented Dec 28, 2022

View reviewed changes

Comment thread daemon/algod/api/server/v2/test/handlers_test.go

e2e test as well

a64a924

tzaffi marked this pull request as ready for review December 29, 2022 04:53

tzaffi requested review from algochoi, algorandskiy and jannotti December 29, 2022 05:00

algochoi reviewed Dec 29, 2022

View reviewed changes

Comment thread cmd/goal/application.go Outdated

algorandskiy reviewed Dec 29, 2022

View reviewed changes

remove incorrect comments

7a7ff1f

tzaffi mentioned this pull request Jan 1, 2023

Enhancement: Small Improvement for Interactively Overriding Local Testing Harness Env Vars algorand/py-algorand-sdk#429

Closed

8 tasks

rename ReportProblems -> MultipleErrors and modify it for the case of…

dac1023

… 1 error to _not_ report the actual errors, but "1 error" instead

tzaffi commented Jan 2, 2023

View reviewed changes

Zeph Grunschlag added 4 commits January 1, 2023 19:35

parallelize

aaf63bf

Let the version check happen before the emptiness check

26c0945

revert version check ordering

2072cab

bundle the original error in as well

cb6308b

tzaffi commented Jan 2, 2023

View reviewed changes

Comment thread test/scripts/e2e_subs/e2e-app-simple.sh Outdated

Comment thread test/scripts/e2e_subs/e2e-app-simple.sh Outdated

tzaffi added 2 commits January 1, 2023 22:30

Update test/scripts/e2e_subs/e2e-app-simple.sh

37c97b1

Update test/scripts/e2e_subs/e2e-app-simple.sh

2241126

Zeph Grunschlag added 3 commits January 1, 2023 23:07

revert

7d14d87

Merge remote-tracking branch 'origin/master' into improve-some-goal-e…

b30ca2f

…rrors

Merge branch 'master' into improve-some-goal-errors

3c91244

algorandskiy reviewed Jan 11, 2023

View reviewed changes

implement suggestion per CR

5f7de10

algorandskiy previously approved these changes Jan 11, 2023

View reviewed changes

revert: don't pass trimmed to the rest of the assembler

7db01b5

tzaffi dismissed algorandskiy’s stale review via 7db01b5 January 11, 2023 20:52

bbroder-algo reviewed Jan 12, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into improve-some-goal-e…

5e4f0ea

…rrors

algorandskiy approved these changes Jan 13, 2023

View reviewed changes

algorandskiy merged commit 4631571 into algorand:master Jan 13, 2023

winder pushed a commit to winder/go-algorand that referenced this pull request Jan 18, 2023

goal: TEAL assembler error UX improvement (algorand#4951)

6d9b7dc

Changed assembler functionality so that logic.assemble errors when receiving empty input

This was referenced Jan 23, 2023

go-algorand 3.14.0-beta Release PR #5043

Merged

go-algorand 3.14.1-beta Release PR #5055

Merged

This was referenced Jan 30, 2023

go-algorand 3.14.1-stable Release PR #5076

Merged

go-algorand 3.14.2-stable Release PR #5110

Merged

tzaffi mentioned this pull request Feb 17, 2023

Interactive env var overrides tzaffi/py-algorand-sdk#3

Open

8 tasks

-	if strings.TrimSpace(text) == "" {
-		return ops.errorf("Cannot assemble empty program text")
-	}
+	// version check
+	...
+	trimmed := strings.TrimSpace(text)
+	if trimmed == "" {
+		return ops.errorf("Cannot assemble empty program text")
+	}
+	fin := strings.NewReader(trimmed)

Conversation

tzaffi commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Question for Consideration

Issues

Test Plan

Uh oh!

codecov Bot commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

algochoi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

algorandskiy Dec 29, 2022

Choose a reason for hiding this comment

Uh oh!

tzaffi Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannotti Dec 30, 2022

Choose a reason for hiding this comment

Uh oh!

tzaffi Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

tzaffi Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tzaffi Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

algorandskiy Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

tzaffi Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

tzaffi Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tzaffi commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbroder-algo Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

tzaffi Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tzaffi commented Dec 28, 2022 •

edited

Loading

codecov Bot commented Dec 28, 2022 •

edited

Loading

tzaffi Dec 29, 2022 •

edited

Loading

tzaffi Jan 2, 2023 •

edited

Loading

tzaffi Jan 2, 2023 •

edited

Loading

tzaffi Jan 11, 2023 •

edited

Loading

tzaffi commented Jan 11, 2023 •

edited

Loading