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

Add PaymentRequest and Payment messages #61

Conversation

x1m3
Copy link
Contributor

@x1m3 x1m3 commented Oct 25, 2024

No description provided.


const (
// PaymentRequestMessageType is a Iden3PaymentMessage payment type
PaymentRequestMessageType iden3comm.ProtocolMessage = iden3comm.DidCommProtocol + "credentials/0.1/payment-request"
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be under Iden3Protocol domain name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also changed PaymentMessageType to same Iden3Protocol domain.


// EthereumEip712Signature2021 represents the Ethereum EIP712 signature.
type EthereumEip712Signature2021 struct {
Type string `json:"type"`
Copy link
Contributor

Choose a reason for hiding this comment

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

verifiable.ProofType

@vmidyllic
Copy link
Contributor

do not merge til erc20 payment spec, please

cc: @volodymyr-basiuk

@x1m3 x1m3 requested a review from vmidyllic October 29, 2024 09:30
Comment on lines 363 to 381
func (p *PaymentContext) UnmarshalJSON(data []byte) error {
var str string
var strCol []string
var itemCol []interface{}

if err := json.Unmarshal(data, &str); err == nil {
p.str = &str
return nil
}
if err := json.Unmarshal(data, &strCol); err == nil {
p.strCol = strCol
return nil
}
if err := json.Unmarshal(data, &itemCol); err == nil {
p.itemCol = itemCol
return nil
}
return errors.Errorf("failed to unmarshal PaymentContext: %s", string(data))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue I see here is that if we would try to unmarshal the input json into already initialized object, we would not fullfill that constraint that only one field should be non-empty. What do you think about more explicit unmarshaling by explicitly checking json types instead of base our logic on returned errors?

Suggested change
func (p *PaymentContext) UnmarshalJSON(data []byte) error {
var str string
var strCol []string
var itemCol []interface{}
if err := json.Unmarshal(data, &str); err == nil {
p.str = &str
return nil
}
if err := json.Unmarshal(data, &strCol); err == nil {
p.strCol = strCol
return nil
}
if err := json.Unmarshal(data, &itemCol); err == nil {
p.itemCol = itemCol
return nil
}
return errors.Errorf("failed to unmarshal PaymentContext: %s", string(data))
}
func (p *PaymentContext) UnmarshalJSON(data []byte) error {
var o any
err := json.Unmarshal(data, &o)
if err != nil {
return err
}
switch v := o.(type) {
case string:
p.str = &v
p.strCol = nil
p.itemCol = nil
case []any:
p.str = nil
p.itemCol = nil
p.strCol = make([]string, len(v))
for i := range v {
s, ok := v[i].(string)
if !ok {
p.strCol = nil
p.itemCol = v
break
}
p.strCol[i] = s
}
default:
return errors.Errorf("failed to unmarshal PaymentContext: %s",
string(data))
}
return nil
}

Copy link
Contributor Author

@x1m3 x1m3 Oct 29, 2024

Choose a reason for hiding this comment

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

I agree. I like your proposal. Let me change it.

Done,

@x1m3 x1m3 requested a review from olomix October 30, 2024 11:58
Recipient string `json:"recipient"`
Amount string `json:"amount"` // Not negative number
ExpirationDate string `json:"expirationDate"`
Proof EthereumEip712Signature2021Col `json:"proof"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I have one doubt regarding that. From the protocol persperctive we can have different proof types here. So we can design structure not to break interfaces later.

What do you think @x1m3 @volodymyr-basiuk ?

Copy link
Contributor

Choose a reason for hiding this comment

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

also it can be an object, not always an array

Copy link
Contributor Author

@x1m3 x1m3 Nov 4, 2024

Choose a reason for hiding this comment

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

Yes. I agree. We need to handle objects and arrays and change Proof structure in a way that can be extended in the future.

Copy link
Contributor Author

@x1m3 x1m3 Nov 4, 2024

Choose a reason for hiding this comment

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

@vmidyllic Internal representation is a collection, but EthereumEip712Signature2021Col is implementing the json unmarshal interface and accepts and object. Output is always a list. So, it is working with single objects or lists.

Naming is not the best.

Regarding diferent ProfTypes we can always change implementation later but, let me try to do a proposal hidding this prooftype into a superobject

Copy link
Contributor Author

@x1m3 x1m3 Nov 4, 2024

Choose a reason for hiding this comment

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

Fixed. Changed it to class that can be extended in the future with other proof types.

@x1m3 x1m3 requested a review from vmidyllic November 4, 2024 15:57
dataType string
crypto []Iden3PaymentRequestCryptoV1
rails []Iden3PaymentRailsRequestV1
railsERC []Iden3PaymentRailsERC20RequestV1
Copy link
Contributor

Choose a reason for hiding this comment

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

let's call it railsERC20

// Iden3PaymentCryptoV1 represents the Iden3PaymentCryptoV1 payment data.
type Iden3PaymentCryptoV1 struct {
ID string `json:"id"`
Type string `json:"type"`
Copy link
Contributor

@volodymyr-basiuk volodymyr-basiuk Nov 5, 2024

Choose a reason for hiding this comment

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

Could we define separate type - iden3comm.PaymentType/PaymentRequestType or similar ?
For this fild Type only one value are possible - Iden3PaymentCryptoV1.
The same for Iden3PaymentRailsV1/Request and Iden3PaymentRailsERC20V1/Request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

vmidyllic
vmidyllic previously approved these changes Nov 6, 2024
@x1m3 x1m3 merged commit c19fef9 into main Nov 8, 2024
5 checks passed
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