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

algod: Add a sourcemap flag for compile endpoint #3938

Merged
merged 15 commits into from
May 13, 2022

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Apr 29, 2022

Summary

Related to #3725:

It would also be nice if this was supported in the REST endpoint as well with an optional parameter, e.g. ?map

This PR adds an optional query parameter to optionally output source maps as a JSON in the CompileResponse. This changes the API request and response structure to include these optional fields.

Test Plan

Added test cases in the handler for optional query parameter and optional source map returned.

Tested on sandbox with following query: curl "localhost:4001/v2/teal/compile?sourcemap=include" -H "X-Algo-API-Token:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" --data-binary @simple.teal

Response:

{"hash":"CMBZH57GLMDRURFNVS47PYWWWVRN2RJKRDL7PISE56AEVF4MUUH4OIESKE","result":"BSABASJEIkM=","sourcemap":{"mapping":";;;;AACA;AAEA;AAGA;AAIA","names":[],"sources":[],"version":3}}

@algochoi algochoi changed the title WIP: Add a sourcemap flag for compile endpoint algod: Add a sourcemap flag for compile endpoint May 9, 2022
@algochoi algochoi self-assigned this May 9, 2022
@algochoi algochoi marked this pull request as ready for review May 9, 2022 19:11
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Nice! I'm happy to see this get support in the REST API as well

daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/test/handlers_test.go Outdated Show resolved Hide resolved
if !v2.Node.Config().EnableDeveloperAPI {
return ctx.String(http.StatusNotFound, "/teal/compile was not enabled in the configuration file by setting the EnableDeveloperAPI to true")
}
// Check if we should return the source map.
sourcemapFlag := false
var sourcemap logic.SourceMap
Copy link
Contributor

@barnjamin barnjamin May 9, 2022

Choose a reason for hiding this comment

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

nitty but i prefer grouping multiple var declarations

var (
 sourcemapFlag bool
 sourcemap logic.SourcceMap
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored the whole section to remove some ugly code

data := rec.Body.Bytes()
err = protocol.DecodeJSON(data, &response)
require.NoError(t, err, string(data))
if params.Sourcemap != nil && *params.Sourcemap {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would make the condition expectedSourcemap != nil, that way you aren't replicating the same logic that determines when a sourcemap should be returned or not

Also, require.Equal(t, expectedSourcemap, response.Sourcemap) compares *logic.SourceMap, does this pointer comparison compare the actual values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch thanks!

jasonpaulos
jasonpaulos previously approved these changes May 10, 2022
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Pending tests passing, looks good to me!

barnjamin
barnjamin previously approved these changes May 10, 2022
@algochoi algochoi dismissed stale reviews from barnjamin and jasonpaulos via 3221002 May 11, 2022 13:56
@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

Merging #3938 (3221002) into master (bd18d04) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master    #3938   +/-   ##
=======================================
  Coverage   49.79%   49.79%           
=======================================
  Files         409      409           
  Lines       69145    69155   +10     
=======================================
+ Hits        34429    34437    +8     
- Misses      30997    31002    +5     
+ Partials     3719     3716    -3     
Impacted Files Coverage Δ
daemon/algod/api/server/v2/handlers.go 0.00% <0.00%> (ø)
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
network/wsNetwork.go 62.79% <0.00%> (-0.20%) ⬇️
catchup/service.go 68.88% <0.00%> (ø)
network/wsPeer.go 67.77% <0.00%> (+1.94%) ⬆️
cmd/algoh/blockWatcher.go 80.95% <0.00%> (+3.17%) ⬆️
ledger/roundlru.go 96.22% <0.00%> (+5.66%) ⬆️

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 bd18d04...3221002. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants