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

Require salt in Rfq privateData JSON schema #306

Merged
merged 3 commits into from
Mar 31, 2024
Merged

Require salt in Rfq privateData JSON schema #306

merged 3 commits into from
Mar 31, 2024

Conversation

diehuxx
Copy link
Contributor

@diehuxx diehuxx commented Mar 31, 2024

@KendallWeihe pointed out that we should make salt a required field in Rfq.privateData because the privateData is impossible to validate -- and therefore useless -- without the salt.

TODO: validate in at least two implementations:

  • tbdex-js PR
  • tbdex-kt PR
  • tbdex-swift

@KendallWeihe
Copy link
Contributor

Should salt have required set to Y in the spec README? (screen shot below)

Screenshot 2024-03-31 at 3 46 25 PM

Copy link
Contributor

@KendallWeihe KendallWeihe left a comment

Choose a reason for hiding this comment

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

one comment needs addressing but good to go after that ✅

Copy link
Contributor

@kirahsapong kirahsapong left a comment

Choose a reason for hiding this comment

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

one comment but lgtm

| field | data type | required | description |
| -------- | ------------------------------------------------- | -------- | ------------------------------------------------------------------------------------- |
| `salt` | string | N | |
| `salt` | string | Y | Randomly generated salt used for hashing PrivateData fields |
Copy link
Contributor

Choose a reason for hiding this comment

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

nice. the rfq json example below this table will need updating too

Copy link
Contributor

Choose a reason for hiding this comment

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

this example too actually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. ty

@diehuxx diehuxx merged commit 38cf284 into main Mar 31, 2024
@diehuxx diehuxx deleted the require-salt branch March 31, 2024 21:36
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.

4 participants