Skip to content
This repository has been archived by the owner on Dec 12, 2024. It is now read-only.

Hash private RFQ fields #12

Merged
merged 5 commits into from
Apr 10, 2024
Merged

Hash private RFQ fields #12

merged 5 commits into from
Apr 10, 2024

Conversation

diehuxx
Copy link

@diehuxx diehuxx commented Apr 10, 2024

Add privateData to RFQ and make exceptions more precise and easier to catch. From the dart docs for the Exception class,

Creating instances of Exception directly with Exception("message") is discouraged in library code since it doesn't give users a precise type they can catch. It may be reasonable to use instances of this class in tests or during development.

The pattern of "error codes" is borrowed from dwn-sdk-js. The pattern of custom error classes -- TbdexParseException, TbdexValidatorException, etc. -- makes it easier to catch specific exceptions from an action like .parse() or .validate().

@@ -0,0 +1,61 @@
// Tbdex exception codes are each thrown in exactly one place.
// Each code is prefixed with the name of the file in which it is thrown.
Copy link
Author

@diehuxx diehuxx Apr 10, 2024

Choose a reason for hiding this comment

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

Not every code is tested as of this PR. In subsequent PRs, we can flesh out tests to cover more cases.

};
}

static Future<Rfq> parse(String rawMessage, { requireAllPrivateData = false }) async {

Choose a reason for hiding this comment

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

In tbdex-js we only use this functionality in the server, so swift doesn't implement anything that isn't consumed by the client, and scopes anything needed for testing to the test files. Do you want to follow the same pattern here?

My suggestion is to:

  • omit parse, verifyAllPrivateData and verifyPresentPrivateData as these are not consumed by a client
  • collapse verifyPayinDetailsHash, verifyPayoutDetailsHash, and verifyClaimsHash into a verifyHash that accepts args
  • scope verifyHash to tests

Copy link
Author

Choose a reason for hiding this comment

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

I'd prefer to keep the requireAllPrivateData functionality. My understanding for tbdex-swift was that we omitted that functionality because we were up against a deadline and omitting it from swift was a good trade off to cut down on eng work.

Since I've already paid the upfront cost of implementing this is dart, I don't see any drawback to keeping it in. Even though we won't necessarily use that functionality for didpay, other package consumers might find it useful for their dart apps. E.g. someone writes p2p tbdex client in dart that has both pfi and wallet functionality.

Choose a reason for hiding this comment

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

My understanding for tbdex-swift was that we omitted that functionality because we were up against a deadline and omitting it from swift was a good trade off to cut down on eng work.

cool thanks for clarifying @diehuxx

@frankhinek @angiejones can you confirm? thats not what my understanding was and it would be ideal to align sdks semantically. if we are adding pfi functionality broadly including in swift i'd like to create issues asap.


abstract class Parser {
static Message parseMessage(String rawMessage) {
final jsonObject = jsonDecode(rawMessage) as Map<String, dynamic>?;
if (jsonObject == null) {
throw Exception('ugh');
Copy link
Contributor

Choose a reason for hiding this comment

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

should we throw a TbdexParseException here?

Copy link
Author

@diehuxx diehuxx Apr 10, 2024

Choose a reason for hiding this comment

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

woops, we definitely should. fixed

Copy link
Contributor

@ethanwlee ethanwlee left a comment

Choose a reason for hiding this comment

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

thanks for getting to this!!

Diane Huxley added 4 commits April 10, 2024 12:05
* main:
  roll back path dep req from `1.9.0` to `1.8.3` (#13)
  fix: payin/payout deserialization (#11)
  re-export lib (#10)
  feat: add http client (#9)
@diehuxx diehuxx merged commit c80223f into main Apr 10, 2024
1 check passed
@diehuxx diehuxx deleted the rfq-private-salt branch April 10, 2024 21:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants