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

Update main.go #729

Closed
wants to merge 3 commits into from
Closed

Update main.go #729

wants to merge 3 commits into from

Conversation

pipaman
Copy link

@pipaman pipaman commented Jan 21, 2020

Program bytes are needed for the Sign function. It doesn't work with 4 parameters in this way since it doesn't verify when you call ed25519verify from teal. With this change, it does verify.
There is another issue I've noticed: when you pass a file as the data to sign, it is likely to have a special character in it, so the final signature includes this special character. I suggest making a string parameter version to avoid this problem.

Summary

Explain the goal of this change and what problem it is solving.

Test Plan

How did you test these changes? Please provide the exact scenarios you tested in as much detail as possible including commands, output and rationale.

Program bytes are needed for the Sign function
@claassistantio
Copy link

claassistantio commented Jan 21, 2020

CLA assistant check
All committers have signed the CLA.

@justicz
Copy link
Contributor

justicz commented Jan 21, 2020

It looks like this change makes it so dsign takes in the source code of a TEAL program, whereas right now it takes in a compiled TEAL program (as produced by goal clerk compile foo.logic). I don't think the current behavior is wrong, but perhaps it should be better documented.

Compilation correction
@pipaman
Copy link
Author

pipaman commented Jan 23, 2020

The problem is that you can't use the output on a TEAL code to call ed25519verify. Maybe, we'll need another option of the same tool that generates the signature of the compiled program that can be used in a TEAL code.

@@ -62,8 +62,13 @@ func main() {
ddata, err := ioutil.ReadFile(os.Args[3])
failFast(err)

programstr := fmt.Sprintf("%s", pdata)

Choose a reason for hiding this comment

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

Why using fmt.Sprintf where fmt.Sprint would do the work (notice no f in function name)?

Copy link
Author

Choose a reason for hiding this comment

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

I'm using Sprint because I've got a byte[] containing a string (the code) and I want to create a string. The alternative is to use:
programstr := string(pdata)

Your proposed change doesn't work because it converts the byte[] to a string containing the binary codes (e.g.: '27 34 93 59') instead of a string with the TEAL code.

Simplified with typecast instead of Sprintf
@derbear
Copy link
Contributor

derbear commented Jan 28, 2020

I'm of the opinion that the dsign program should just sign bytes and not include an entire TEAL assembler, which is redundant with the goal clerk compile command. I do agree with @justicz that the documentation here is lacking.

@derbear
Copy link
Contributor

derbear commented Feb 11, 2020

@pipaman, we decided to take your suggestion and integrate this functionality into goal clerk tealsign. How does #800 look?

@pipaman
Copy link
Author

pipaman commented Feb 12, 2020 via email

@justicz justicz closed this Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants