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

[sBTC] 3553 add burn ops rpc for pegout #3576

Conversation

stjepangolemac
Copy link
Contributor

@stjepangolemac stjepangolemac commented Feb 17, 2023

Description

Uses a temporary branch as a base until some prerequisite work is merged into next.

Applicable issues

  • adds the PegOut RPC

Checklist

  • Test coverage for new or modified code paths (@netrome seems some related tests were added in the peg out wire format PR, do we need additional tests?)

@CLAassistant
Copy link

CLAassistant commented Feb 17, 2023

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

❗ No coverage uploaded for pull request base (3553-add-burn-ops-rpc-for-pegout-prerequisits@8e4672b). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 855470e differs from pull request most recent head 181dac8. Consider uploading reports for the commit 181dac8 to get more accurate results

@@                               Coverage Diff                                @@
##             3553-add-burn-ops-rpc-for-pegout-prerequisits    #3576   +/-   ##
================================================================================
  Coverage                                                 ?   27.93%           
================================================================================
  Files                                                    ?      309           
  Lines                                                    ?   277756           
  Branches                                                 ?        0           
================================================================================
  Hits                                                     ?    77595           
  Misses                                                   ?   200161           
  Partials                                                 ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@stjepangolemac
Copy link
Contributor Author

@kantai do we have any tests for API responses? This diff seems suspiciously small and that's the only thing that crossed my mind, so I might've missed it. Also, let me know if you find that something is missing.

@netrome netrome requested a review from kantai February 17, 2023 13:21
@netrome
Copy link
Contributor

netrome commented Feb 17, 2023

We also should extend the test_submit_and_observe_sbtc_ops test in neon_integrations.rs. This won't test the serialized format of API responses though, but at least verifies that the API picks up new peg out requests and peg out fulfillments.

Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. One request, can we add a serialization test similar to what Aaron added here https://github.com/stacks-network/stacks-blockchain/pull/3511/files#diff-0cc1d8499a6219cd0467abfddd7ae0eb6e5269b10df975cf68d79b35d70867e4?

@stjepangolemac
Copy link
Contributor Author

I'll need to create another prereq branch from the latest parent branches:

  • feat/sbtc-burn-ops-rpc
  • 3493-sbtc-peg-out-wire-format

@jcnelson
Copy link
Member

If it's for sBTC, please label it as such. Thanks!

@netrome netrome changed the title 3553 add burn ops rpc for pegout [sBTC] 3553 add burn ops rpc for pegout Feb 21, 2023
@netrome
Copy link
Contributor

netrome commented Feb 21, 2023

If it's for sBTC, please label it as such. Thanks!

Done. This is to be incorporated in #3577 since the changes introduced here are minimal and closely related to it. I'm closing this PR since we don't intend to merge this one.

@netrome netrome closed this Feb 21, 2023
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

7 participants