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

rfq.VerifyOfferingRequirements() #51

Merged
merged 13 commits into from
Jun 12, 2024
Merged

Conversation

jiyoonie9
Copy link
Contributor

@jiyoonie9 jiyoonie9 commented Jun 4, 2024

  • Implement rfq.verifyOfferingRequirements
  • Wrote a lot of tests against this new impl
  • While testing, I found that offering.create() didn't always work because it was missing some bits like applying options, or missing a field, so I fixed those as well.

@jiyoonie9 jiyoonie9 linked an issue Jun 4, 2024 that may be closed by this pull request
@jiyoonie9 jiyoonie9 marked this pull request as ready for review June 5, 2024 05:01
tbdex/rfq/rfq.go Outdated Show resolved Hide resolved
tbdex/rfq/rfq.go Outdated Show resolved Hide resolved
@@ -47,9 +47,35 @@ type PayoutDetails struct {
Methods []PayoutMethod `json:"methods,omitempty"`
}

// PaymentMethod is an interface for PayinMethod and PayoutMethod.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i declared an interface for both PayinMethod and PayoutMethod to snap to, just so that i can write this:

func (r *RFQ) verifyPaymentMethod(
	selectedPaymentKind string,
	selectedPaymentDetailsHash string,
	privateData *PrivateData,
	offeringPaymentMethods []offering.PaymentMethod, // <- this part
	payDirection PayDirection,
) error

without this, i would have had to resort to writing this function twice, one for verifyPayinMethod that passes in offeringPayinMethods []offering.PayinMethod and verifyPayoutMethod that passes in offeringPayoutMethods []offering.PayoutMethod

go does not support type inheritance and this was my best attempt at it. open to other suggestions tho

tbdex/rfq/rfq.go Outdated
Comment on lines 187 to 190
payinAmount, err := strconv.ParseFloat(r.Data.Payin.Amount, 64)
if err != nil {
return fmt.Errorf("failed to parse payin amount: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

we should use this decimal lib instead of strconv.ParseFloat

tbdex/rfq/rfq.go Outdated
Comment on lines 193 to 196
maxAmount, err := strconv.ParseFloat(offering.Data.Payin.Max, 64)
if err != nil {
return fmt.Errorf("failed to parse offering payin max amount: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

we should use this decimal lib instead of strconv.ParseFloat

tbdex/rfq/rfq.go Outdated
Comment on lines 205 to 208
minAmount, err := strconv.ParseFloat(offering.Data.Payin.Min, 64)
if err != nil {
return fmt.Errorf("failed to parse offering payin min amount: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

we should use this decimal lib instead of strconv.ParseFloat

@mistermoe
Copy link
Member

kk @jiyoontbd i think this PR is ready to review if you want to take a look!

Changes

  • simplified linting rules which unearthed some stuff e.g. more misspelled omitemptys
  • flattened rfq.VerifyOfferingRequirements to prioritize correctness at the cost of a little bit duplication. thinking we can come back later when we have more time to figure out a way to DRY things up
  • updated logic for mins and maxes to prioritize payment method specific min/max if present

@@ -1,123 +0,0 @@
run:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

did i delete this? we need this for linter settings right?

@@ -9,8 +9,14 @@ import (
"github.com/TBD54566975/tbdex-go/tbdex/closemsg"
"github.com/TBD54566975/tbdex-go/tbdex/crypto"
"github.com/TBD54566975/tbdex-go/tbdex/message"
_offering "github.com/TBD54566975/tbdex-go/tbdex/offering"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we need to alias it like _offering here?

}

var min *decimal.Decimal
maybeMins := []string{selectedPayinMethod.Min, offering.Data.Payin.Min}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh no! i see why you want to get rid of Payin.min at the Payin level. it should be for each payment method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe this wasn't what you were talking about but filed an issue

TBD54566975/tbdex#333

@mistermoe mistermoe merged commit fcd986d into main Jun 12, 2024
2 checks passed
@mistermoe mistermoe deleted the 44-rfq-verifyOfferingRequirements branch June 12, 2024 22:31
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.

Implement rfq.VerifyOfferingRequirements
2 participants