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

Hash Rfq privateData #222

Merged
merged 16 commits into from
Apr 1, 2024
Merged

Hash Rfq privateData #222

merged 16 commits into from
Apr 1, 2024

Conversation

diehuxx
Copy link

@diehuxx diehuxx commented Mar 29, 2024

Implement min bar changes laid out in this spec change: TBD54566975/tbdex#294
In further PRs, we could improve ergonomics around validating/disclosing specific privateData fields.

See also equivalent PR in tbdex-js: TBD54566975/tbdex-js#215

@jiyoonie9 jiyoonie9 linked an issue Mar 29, 2024 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2024

Codecov Report

Merging #222 (da99f4c) into main (bd505a5) will increase coverage by 4.95%.
Report is 5 commits behind head on main.
The diff coverage is 70.68%.

❗ Current head da99f4c differs from pull request most recent head 5d46ac0. Consider uploading reports for the commit 5d46ac0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #222      +/-   ##
==========================================
+ Coverage   79.39%   84.34%   +4.95%     
==========================================
  Files          35       38       +3     
  Lines         820     1124     +304     
  Branches       75      114      +39     
==========================================
+ Hits          651      948     +297     
+ Misses        133      124       -9     
- Partials       36       52      +16     
Components Coverage Δ
protocol 85.85% <81.81%> (-2.16%) ⬇️
httpclient 78.34% <70.45%> (-2.48%) ⬇️

@jiyoonie9 jiyoonie9 linked an issue Mar 29, 2024 that may be closed by this pull request
@diehuxx diehuxx mentioned this pull request Mar 30, 2024
@diehuxx diehuxx force-pushed the rfq-private-salt branch 3 times, most recently from 428c714 to 00482eb Compare March 30, 2024 01:55
Base automatically changed from 207-protocol-spec-changes to main March 30, 2024 05:10
@diehuxx diehuxx changed the title WIP hash Rfq privateData Hash Rfq privateData Mar 31, 2024
@diehuxx diehuxx marked this pull request as ready for review March 31, 2024 21:35
Copy link
Author

@diehuxx diehuxx left a comment

Choose a reason for hiding this comment

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

Offline, @mistermoe @jiyoontbd @KendallWeihe @kirahsapong and I discussed a DX improvement:

Rename classes with the Unhashed* prefix to instead of Create* prefix, e.g. UnhashedRfqData -> CreateRfqData and UnhashedPayinMethod -> CreatePayinMethod.

We came to this decision because we believe that introducing the concept of hashing when sdk consumer is trying to Rfq.create() causes confusion, and hashing should be an implementation detail. If they choose, the developer can learn more about hashing through either the ktdocs of the relevant types like RfqData or RfqPrivateData, or the tbdex protocol spec, which outlines the structure of the Rfq returned from calling create()

I'll push a commit for this in juuuust a sec.

Copy link
Contributor

@jiyoonie9 jiyoonie9 left a comment

Choose a reason for hiding this comment

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

couple nits for doc, not blocking. well done 🎉

@diehuxx diehuxx merged commit 85598e1 into main Apr 1, 2024
6 checks passed
@diehuxx diehuxx deleted the rfq-private-salt branch April 1, 2024 16:16
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.

Make CI pass after merging #211 Specify structure of private fields in RFQ
4 participants