Skip to content

feat: allow parsing Bitcoin deposit memo with inscription and add E2E test for parsing#2727

Closed
bitSmiley wants to merge 48 commits intozeta-chain:developfrom
bitSmiley:e2e
Closed

feat: allow parsing Bitcoin deposit memo with inscription and add E2E test for parsing#2727
bitSmiley wants to merge 48 commits intozeta-chain:developfrom
bitSmiley:e2e

Conversation

@bitSmiley
Copy link
Contributor

@bitSmiley bitSmiley commented Aug 16, 2024

Description

Adding e2e test for detecting memo from inscription.

The current btcutil does not have enough tapscript support, in this case, a nodejs sidecar to handle inscription txn generation is used instead. This is not intrusive and can be quickly removed once btcutil is upgraded.

How Has This Been Tested?

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Summary by CodeRabbit

  • New Features

    • Introduced a logging feature to enhance visibility of test executions.
    • Added a test case for extracting Bitcoin inscription memo names.
    • Implemented an Express.js server with endpoints for Bitcoin transaction handling.
    • Developed a ScriptBuilder for constructing tapscripts used in Bitcoin transactions.
    • Created a utility function for formatting public keys in Bitcoin transactions.
  • Bug Fixes

    • Improved transaction processing by updating methods to include witness data for better validation.
  • Documentation

    • Added configuration files like tsconfig.json for TypeScript setup.
  • Chores

    • Updated Docker configurations to include a new Bitcoin sidecar service in the setup.

Co-authored-by: Lucas Bertrand <lucas.bertrand.22@gmail.com>
@lumtis
Copy link
Contributor

lumtis commented Aug 21, 2024

It seems it breaks the deposit BTC test: https://github.com/zeta-chain/node/actions/runs/10471358149/job/28998509065

Is the change supposed to be backward compatible

@bitSmiley
Copy link
Contributor Author

@lumtis No, it should be backward compatible. There is an error fixed in b341832, which if OP_RETURN and inscription both cannot find the memo, then it should just return nil not error.

bitSmiley and others added 4 commits August 21, 2024 20:53
Co-authored-by: Lucas Bertrand <lucas.bertrand.22@gmail.com>
Co-authored-by: Lucas Bertrand <lucas.bertrand.22@gmail.com>
@bitSmiley bitSmiley requested a review from lumtis August 21, 2024 13:36
@lumtis
Copy link
Contributor

lumtis commented Aug 21, 2024

@lumtis No, it should be backward compatible. There is an error fixed in b341832, which if OP_RETURN and inscription both cannot find the memo, then it should just return nil not error.

Ok, rerunning

@bitSmiley
Copy link
Contributor Author

@lumtis sorry man, I just fixed the linting and change log, please help run the pipeline again.

@lumtis lumtis marked this pull request as draft August 27, 2024 09:04
@lumtis lumtis marked this pull request as ready for review September 14, 2024 00:14
@lumtis
Copy link
Contributor

lumtis commented Sep 14, 2024

Opening back for develop for the next release

@lumtis
Copy link
Contributor

lumtis commented Oct 2, 2024

Closing in favor of #2957 as discussed on Slack

@lumtis lumtis closed this Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking:cli UPGRADE_TESTS Run make start-upgrade-tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants