Skip to content

Conversation

@carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Mar 21, 2025

Changelog

- description: |
    Add canonical CBOR output toggle for transaction building and signing commands
# uncomment types applicable to the change:
  type:
   - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
   - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Requires:

Implements:

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@carbolymer carbolymer self-assigned this Mar 21, 2025
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM but three things:

  • Let's have a separate function for the ordered tx body
  • Let's not call it canonical CBOR. Maybe writeOrderedTx or something like that
  • Instead of Bool use a sum type.

@carbolymer carbolymer force-pushed the mgalazyn/feature/add-canonical-cbor-tx branch 2 times, most recently from a43f8e2 to c9626a0 Compare March 27, 2025 14:20
@carbolymer carbolymer marked this pull request as ready for review March 27, 2025 14:29
@carbolymer carbolymer force-pushed the mgalazyn/feature/add-canonical-cbor-tx branch from bf38b52 to 81ede0a Compare April 4, 2025 17:29
@carbolymer carbolymer requested a review from a team April 4, 2025 17:29
@carbolymer carbolymer requested a review from a team as a code owner April 4, 2025 17:29
@carbolymer carbolymer force-pushed the mgalazyn/feature/add-canonical-cbor-tx branch from 81ede0a to 1ee6145 Compare April 4, 2025 17:31
@carbolymer carbolymer requested a review from Jimbo4350 April 4, 2025 17:32
@carbolymer carbolymer force-pushed the mgalazyn/feature/add-canonical-cbor-tx branch from 1ee6145 to c6e26aa Compare April 8, 2025 11:06
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM! A few minor comments to address.

@carbolymer carbolymer force-pushed the mgalazyn/feature/add-canonical-cbor-tx branch from c6e26aa to e186c2e Compare April 11, 2025 15:53
@carbolymer carbolymer enabled auto-merge April 11, 2025 15:54
@carbolymer carbolymer force-pushed the mgalazyn/feature/add-canonical-cbor-tx branch from e186c2e to 1423ce1 Compare April 15, 2025 08:58
@carbolymer carbolymer added this pull request to the merge queue Apr 15, 2025
Merged via the queue into master with commit d70bb88 Apr 15, 2025
25 checks passed
@carbolymer carbolymer deleted the mgalazyn/feature/add-canonical-cbor-tx branch April 15, 2025 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FR] - Flag for hardware wallet compatibility when building transactions

3 participants