Skip to content

Comments

Refactor: Commands as traits + new Card construction#12

Merged
notmandatory merged 1 commit intobitcoindevkit:masterfrom
brh28:refactor-traits
Jun 2, 2023
Merged

Refactor: Commands as traits + new Card construction#12
notmandatory merged 1 commit intobitcoindevkit:masterfrom
brh28:refactor-traits

Conversation

@brh28
Copy link
Collaborator

@brh28 brh28 commented Jun 1, 2023

Description

  • Breaking apart SharedCommands trait to encapsulate command logic and improve card composability
  • Defined trait commands into commands.rs and moved command/response apdu into adpu.rs
  • New as_cktap function in Transport trait + impl Transport on pcsc Card

Notes to the reviewers

Just read the CONTRIBUTING.md. I'll be more mindful of it moving forward, but hopefully it's not too much of an issue this time around.

Changelog notice

Checklists

All Submissions:

@brh28 brh28 marked this pull request as draft June 1, 2023 13:23
@brh28 brh28 force-pushed the refactor-traits branch from 28d4d5f to 2520f71 Compare June 1, 2023 17:12
as_cktap on transport trait + impl Transport for Card

Fix formatting

Fix clippy warnings
@brh28 brh28 force-pushed the refactor-traits branch from 2520f71 to 04c94d4 Compare June 1, 2023 21:24
@brh28 brh28 marked this pull request as ready for review June 1, 2023 21:26
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 04c94d4

Another good refactor, the layering of apdu messages and new commands traits makes sense. I also like how you simplified the apdu file.

@notmandatory notmandatory merged commit 04c94d4 into bitcoindevkit:master Jun 2, 2023
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.

2 participants