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
Algod compile api #1007
Algod compile api #1007
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one input validation TODO
I guess we need a simple |
@pzbitskiy currently we don't have any unit test for rest API. The tests are done by SDK separately. I plan to add python sdk support as well as test after python sdk support v2 APIs. |
daemon/algod/api/algod.oas2.json
Outdated
@@ -595,7 +595,60 @@ | |||
"required": true | |||
} | |||
] | |||
} | |||
}, | |||
"/v2/compile": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider giving it it's own segment - /v2/teal/compile
and the other one will be /v2/teal/dryrun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the other as /v2/transactions/dryrun , but /v2/teal/... makes sense too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/v2/teal/compile
makes sense. Will change to that.
daemon/algod/api/algod.oas2.json
Outdated
"operationId": "TealCompile", | ||
"parameters": [ | ||
{ | ||
"description": "Teal source code to be compiled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - TEAL
daemon/algod/api/algod.oas3.yml
Outdated
@@ -1,423 +1,643 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this file contains many irrelevant changes. @winder is that the map random order issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have (in an unrelated pull) something that changes this to sorted keys for stable diffs.
This endpoint could unit test pretty easily since it doesn't rely on setting up any node or ledger state, but the endpoint is a pretty thin wrapper on logic.AssembleString() which is well tested. I don't think it needs a unit test. I kinda expect setting up a test to fake up an echo.Context() and call the handler would be annoying. |
@algobolson @pzbitskiy @winder @rotemh I have fixed all the comments and this is ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Brian approved with a different username.
Switched primary accounts, this is the new me. LGTM. I guess we're waiting on @rotemh to see if his concerns have been addressed? |
ping @rotemh |
Summary
New v2 API endpoint for compile teal code:
/v2/compile
POST text/plain utf-8 encoded source code
Return json if succeed
{
“result”:base64 encoded bytes,
“hash”:base32 sha512_256 of program bytes (Address style),
}
Otherwise 400 error (bad request) with actual error message
Resolve #1093
Test Plan
Tested locally.