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

feat: Add Sell Data for Data Market Module #255

Merged
merged 41 commits into from
Feb 4, 2022
Merged

Conversation

inchori
Copy link

@inchori inchori commented Jan 18, 2022

What I done

  • Configure tx.proto file and msg_server_deal.go.
  • Write SellOwnData function in keeper/deal.go .
  • Write Test Code for SellOwnData function.

TODO

  • Verification for DataValidator Signature.
  • Replace max_num_data field and cur_num_data field with pricePerData.
  • Command-Line for SellData for Tx.

This branch will be base branch for SellData feature.

@inchori inchori added this to the DataMarket M1 milestone Jan 18, 2022
x/market/keeper/deal.go Outdated Show resolved Hide resolved
x/market/keeper/deal.go Outdated Show resolved Hide resolved
x/market/keeper/deal.go Outdated Show resolved Hide resolved
x/market/keeper/deal.go Show resolved Hide resolved
x/market/keeper/deal.go Outdated Show resolved Hide resolved
x/market/types/keys.go Outdated Show resolved Hide resolved
Copy link
Contributor

@gyuguen gyuguen left a comment

Choose a reason for hiding this comment

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

Thank you. I put a few comments

x/market/keeper/deal.go Outdated Show resolved Hide resolved
x/market/keeper/deal.go Outdated Show resolved Hide resolved
x/market/keeper/deal.go Outdated Show resolved Hide resolved
x/market/types/keys.go Outdated Show resolved Hide resolved
x/market/types/message_deal.go Show resolved Hide resolved
Copy link
Contributor

@0xHansLee 0xHansLee left a comment

Choose a reason for hiding this comment

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

Thanks @inchori for your works. I left a few comments more for this pr. If you have any question of thoughts, feel free to mention me!

x/market/keeper/deal.go Outdated Show resolved Hide resolved
x/market/keeper/deal.go Outdated Show resolved Hide resolved
x/market/keeper/deal.go Show resolved Hide resolved
x/market/types/keys.go Outdated Show resolved Hide resolved
x/market/types/message_deal.go Outdated Show resolved Hide resolved
x/market/types/message_deal.go Outdated Show resolved Hide resolved
x/market/keeper/deal_test.go Outdated Show resolved Hide resolved
@inchori
Copy link
Author

inchori commented Jan 24, 2022

@youngjoon-lee @hansol-medi Thanks for yours feedback! Could you review or approve #260 this branch first please? I will merge it to this branch and keep working!

Copy link
Contributor

@youngjoon-lee youngjoon-lee left a comment

Choose a reason for hiding this comment

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

Great job. I think this PR can be merged as long as some comments above are resolved.

@inchori
Copy link
Author

inchori commented Jan 25, 2022

TODO

  • Failure cases test code for not only CreateDeal, but also SellData.
  • Change field name cur_num_data and max_num_data to price_per_data in deactivate-deal feature.

Copy link
Contributor

@0xHansLee 0xHansLee left a comment

Choose a reason for hiding this comment

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

Many reviews have been resolved.
I think you might miss few points and I commented.

x/market/keeper/deal.go Show resolved Hide resolved
x/market/keeper/deal.go Outdated Show resolved Hide resolved
x/market/types/message_deal.go Outdated Show resolved Hide resolved
proto/panacea/market/v2/tx.proto Outdated Show resolved Hide resolved
x/market/client/cli/tx.go Outdated Show resolved Hide resolved
x/market/keeper/deal.go Outdated Show resolved Hide resolved
@@ -119,17 +120,32 @@ func NewSellDataMsg(clientCtx client.Context, txf tx.Factory, fs *flag.FlagSet)
return txf, nil, fmt.Errorf("failed to parse data certificate file: %w", err)
}

encryptedDataUrlBytes, err := base64.URLEncoding.DecodeString(sellData.Cert.UnsignedCert.EncryptedDataUrlBase64)
Copy link
Contributor

Choose a reason for hiding this comment

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

In data validator, base64.StdEncoding is used for encoding encryptedDataUrl.
It seems to use the same encoding method because they use different 2 characters

  • StdEncoding : ABCD...0123456789+/
  • UrlEncoding : ABCD...0123456789-_

Copy link
Contributor

@gyuguen gyuguen Feb 3, 2022

Choose a reason for hiding this comment

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

Oh, I missed this part in the previous PR.
I think base64.UrlEncoding should be written in the URL or file name.
The field EncryptedDataUrlBase64 has a URL in its name, but it is actually encrypted String data, so it seems better to use base64.StdEncoding, a standard Base64 encoding.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't know there have a difference between StdEncoding and UrlEncoding. Thank you!

Copy link
Contributor

@0xHansLee 0xHansLee left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@youngjoon-lee youngjoon-lee left a comment

Choose a reason for hiding this comment

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

Good!

@inchori inchori requested a review from gyuguen February 4, 2022 01:26
app/config.go Outdated
@@ -0,0 +1,61 @@
package app
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to put this commit in the next pr.
This is because new features have already been approvaed by other reviewers before they are uploaded.

@inchori inchori merged commit fd49aba into master Feb 4, 2022
@inchori inchori deleted the ft/na/market/sell-data branch February 4, 2022 05:02
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