Skip to content

feat: parse new output script types#73

Open
r4mmer wants to merge 14 commits intodevfrom
feat/identify-script-type
Open

feat: parse new output script types#73
r4mmer wants to merge 14 commits intodevfrom
feat/identify-script-type

Conversation

@r4mmer
Copy link
Member

@r4mmer r4mmer commented May 28, 2024

Acceptance criteria

  • Identify and parse different output scripts
  • Add support for showing p2sh addresses
  • Add support for showing and confirming data script outputs

@r4mmer r4mmer self-assigned this May 28, 2024
@r4mmer r4mmer changed the title Feat/identify script type feat: parse new output script types May 28, 2024
@r4mmer r4mmer changed the base branch from master to dev May 28, 2024 17:08
r4mmer added 2 commits October 3, 2024 09:06
* origin/dev:
  chore: update linter version
  chore: update CI (#77)
  feat: use correct size limit
  fix: code scanning
  [auto] Add guideline enforcer
  [auto] Add manifest
@r4mmer r4mmer requested a review from pedroferreira1 October 3, 2024 13:31
Co-authored-by: Pedro Ferreira <phsf.pedro@gmail.com>
THROW(TX_STATE_READY);
}
// validate script and extract pubkey hash
validate_p2pkh_script(&buf, script_len);
Copy link
Member

Choose a reason for hiding this comment

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

I didn't find where you moved this validation to. Don't you need it anymore?

#include "opcodes.h"

/**
* Identifies if the given script is P2PKH
Copy link
Member

Choose a reason for hiding this comment

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

Add the expected pattern in the docstring, please.

}

/**
* Identifies if the given script is P2SH
Copy link
Member

Choose a reason for hiding this comment

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

Add the expected pattern in the docstring, please.

return true;
}

bool identify_data_script(buffer_t *in, uint16_t script_len) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing docstring. Also, please add the expected pattern to the docstring.

* Read data from script and store it in `out`
* Returns number of bytes read, 0 if any error occurs.
*/
uint8_t read_data_script(buffer_t *in, uint16_t script_len, uint8_t out[MAX_DATA_SCRIPT_LEN]) {
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find where you use this function. I was also wondering whether it would be easier to have a parser function that would try to parse and return error otherwise. This would remove the necessity to have both identify_data_script() and read_data_script() methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

The identify methods are used to check for valid scripts which is nice to have a separate function for.

It also leaves some possibility to have methods that check for a valid output or transaction without extracting any data.

break;
case SCRIPT_DATA:
out->type = SCRIPT_DATA;
// XXX: We currently do not read the actual data from the output
Copy link
Member

Choose a reason for hiding this comment

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

Don't we present it to the user to confirm? Why not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added support for data output, including an alternative output confirmation for data outputs

@r4mmer r4mmer linked an issue Mar 17, 2025 that may be closed by this pull request
r4mmer added 3 commits March 20, 2025 16:21
* dev:
  feat: codeql triggers (#78)
  fix: stop confirming outputs before parsing them (#70)
  chore: changelog updates and utils (#72)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

[design] Add support for P2SH and data scripts on Ledger

3 participants