Goal: Warn on invalid method signature assembly

[jasonpaulos]

Summary

With this PR, the assembler will now warn if an invalid method signature is used as an argument to the method pseudo-op. This is just a warning so the program will still compile, but it should help users be aware of mistakes they might have made.

No urgency to this PR.

Example

test.teal:

#pragma version 6
method "abc"
pop
method "abc(uint64)"
pop
method "abc(uint64)void"
pop
method "abc(blah)void"
pop
int 1
return

$ goal clerk compile test.teal        
warning: 2: Invalid ARC-4 ABI method signature for method op: No parenthesis in method signature: "abc"
warning: 4: Invalid ARC-4 ABI method signature for method op: Error parsing return type: cannot convert the string "" to an ABI type
warning: 8: Invalid ARC-4 ABI method signature for method op: Error parsing argument type at index 0: cannot convert the string "blah" to an ABI type
3 warnings
test.teal: 43KDA5RKR22UG7M53XNVD7N7CYTS6KI7WRAAZL6MNGVUZIOHLKGLO4VYQ4

Test Plan

Unit test added.

[ahangsu]

LGTM

[jannotti]

Looks good. Let's do something similar for "event" (after spending a little time defining it), and then a bare selector pseudo-op. Once we have those, I think this would becomes an error rather than warning.

Why did you have to suppress printing, rather than separating assembly output from earnings by using stderr?

[codecov-commenter]

Codecov Report

Merging #3614 (3e1226d) into master (af3b5d3) will decrease coverage by 0.03%.
The diff coverage is 19.23%.

@@            Coverage Diff             @@
##           master    #3614      +/-   ##
==========================================
- Coverage   49.88%   49.84%   -0.04%     
==========================================
  Files         392      392              
  Lines       68685    68713      +28     
==========================================
- Hits        34264    34251      -13     
- Misses      30669    30706      +37     
- Partials     3752     3756       +4     

Impacted Files	Coverage Δ
cmd/goal/account.go	12.94% <0.00%> (ø)
cmd/goal/application.go	11.16% <0.00%> (ø)
cmd/goal/clerk.go	9.13% <0.00%> (-0.10%)	⬇️
cmd/goal/commands.go	10.40% <0.00%> (-0.04%)	⬇️
cmd/goal/logging.go	8.92% <0.00%> (ø)
data/abi/abi_encode.go	61.26% <0.00%> (-2.82%)	⬇️
data/abi/abi_type.go	88.62% <100.00%> (ø)
data/transactions/logic/assembler.go	79.96% <100.00%> (+0.05%)	⬆️
cmd/algoh/blockWatcher.go	77.77% <0.00%> (-3.18%)	⬇️
ledger/tracker.go	74.67% <0.00%> (-2.15%)	⬇️
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af3b5d3...3e1226d. Read the comment docs.

[jasonpaulos]

Originally I just used the same convention for warnings as other places in goal, which is to print to stdout. But I agree this seems odd, so I now changed it so that all warnings go to stderr for goal commands.

This is a little more far reaching than I intended, so I hope it doesn't disrupt anything else.

[ahangsu]

Hey folks, I was just reviewing this PR again and was wondering about what JJ previously mentioned:

Let's do something similar for "event" (after spending a little time defining it), and then a bare selector pseudo-op.

What is "event" referring to, does that mean another op/pseudo-op?

[jannotti]

Hey folks, I was just reviewing this PR again and was wondering about what JJ previously mentioned:

Let's do something similar for "event" (after spending a little time defining it), and then a bare selector pseudo-op.

What is "event" referring to, does that mean another op/pseudo-op?

An "event" in ethereum is a structured message that a contract can emit. Our log opcode is intended to cover this use case (in addition to being a "return value" mechanism). Events have a name, and some associated data. Think of an event like "I have transferred 50 algos from address A to address B". It might be named "transfer" and have three fields of type uint64, address and address. So, my strawman proposal is that an app would emit an "event" of this kind by emitting a log message prefixed by hash("transfer(uint64,address,address"). The bytes after the hash would be the ABI encoding of those three values.

[ahangsu]

Uh I see, thanks for explaining! I was also reading: https://medium.com/mycrypto/understanding-event-logs-on-the-ethereum-blockchain-f4ae7ba50378, seems matching what you just mentioned.

I suppose this "event" sounds like a "structured log", which allows for some structured fields filled by ABI encodings, and it might worth it to check the validity of those fields, just like this PR.

[jannotti]

Seems good. Merging.