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.

Sign up for GitHub

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

Jump to bottom

Complete call request #932

Merged
merged 10 commits into from
Feb 1, 2024
Merged

Complete call request #932

merged 10 commits into from
Feb 1, 2024

Conversation

kompotkot
Copy link
Contributor

@kompotkot kompotkot commented Oct 4, 2023 •
edited
Loading

Resolves #930
Resolves #929

For tests sig creation can be used tool: https://github.com/kompotkot/simple-signer

curl 'http://localhost:7191/metatx/requests/<request_id>/complete' \
--header 'Authorization: metatx <signer_base64_message>' \
--form 'tx_hash="<tx_hash>"'

@kompotkot
Copy link
Contributor Author

kompotkot commented Oct 4, 2023 •
edited
Loading

@bugout-dev check

@kompotkot kompotkot marked this pull request as ready for review October 16, 2023 11:55
@zomglings zomglings mentioned this pull request Oct 30, 2023
Open
zomglings
zomglings requested changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@zomglings zomglings left a comment

Choose a reason for hiding this comment

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

I don't see a caller check on the complete_call_request endpoint. That seems to be a serious issue.

engineapi/engineapi/routes/metatx.py Outdated


# @app.post("/requests/{request_id}/complete", tags=["requests"])
@app.post("/requests/complete", tags=["requests"])
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be /requests/<request_id>/complete IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem in whitelist_paths, dynamic <request_id> and our current implementation of authentification middleware. I tried to avoid making this over complicated changes so this is the only way to make this route public.

Copy link
Contributor

Choose a reason for hiding this comment

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

How complex would it be to update our whitelist_paths implementation?

engineapi/engineapi/routes/metatx.py Outdated Show resolved Hide resolved
engineapi/engineapi/routes/metatx.py Outdated Show resolved Hide resolved
zomglings
zomglings reviewed Dec 7, 2023
View reviewed changes
Copy link
Contributor

@zomglings zomglings left a comment

Choose a reason for hiding this comment

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

A few comments.

engineapi/engineapi/version.txt Outdated Show resolved Hide resolved
engineapi/engineapi/routes/metatx.py Outdated Show resolved Hide resolved
engineapi/engineapi/middleware.py Outdated Show resolved Hide resolved
Andrei-Dolgolev
Andrei-Dolgolev approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@Andrei-Dolgolev Andrei-Dolgolev left a comment

Choose a reason for hiding this comment

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

lgtm

@kompotkot kompotkot merged commit 9ebe300 into main Feb 1, 2024
1 check failed
@kompotkot kompotkot deleted the complete-call-request branch February 1, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zomglings zomglings zomglings requested changes

@Andrei-Dolgolev Andrei-Dolgolev Andrei-Dolgolev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a POST /metatx/complete_call_request/<call_request_id> endpoint Add tx_hash field to call_requests
3 participants
@kompotkot @zomglings @Andrei-Dolgolev