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: implementation of MsgBuyDataAccessNFT Tx #306

Merged
merged 25 commits into from
Apr 25, 2022

Conversation

0xHansLee
Copy link
Contributor

Implemented MsgBuyDataAccessNFT Tx, not queries.
As we discussed before, if the data pool is activated, the data access NFT is minted to the buyer. Otherwise, the buyer is added to white list of the pool. They are given the NFT when the pool is activated (maybe gyuguen is working now).

I use incrementing number as a id of NFT(token_id in contract), like stargaze. It is reasonable because each pool has each contract instance, and the id is used in each contract instance.

@0xHansLee 0xHansLee added the Market Data Marketplace for Web3.0 label Apr 13, 2022
@0xHansLee 0xHansLee added this to the Data Pool Model milestone Apr 13, 2022
@0xHansLee 0xHansLee requested a review from inchori April 13, 2022 11:27
@0xHansLee 0xHansLee self-assigned this Apr 13, 2022
Comment on lines 12 to 16
repeated DataValidator data_validators = 1 [(gogoproto.nullable) = false];
uint64 next_pool_number = 2;
repeated Pool pools = 3 [(gogoproto.nullable) = false];
Params params = 4 [(gogoproto.nullable) = false];
repeated WhiteList white_list = 5 [(gogoproto.nullable) = false];
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 also add [ (gogoproto.nullable) = false ] to some fields.

When I export genesis.json with multiple values of those fields, all the same values (I guess the last one) are stored and exported. I referred to other projects and they all used the gogoproto extension. I'm not sure of the reason, it was a workaround for now. If you have any idea, please leave some comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you saying this?
expected:

{
  "whiteList": [
    {
      "data_pool_code_id" : 1,
      "data_pool_nft_contract_adress": "panaceaxxxx1"
    },
    {
      "data_pool_code_id" : 2,
      "data_pool_nft_contract_adress": "panaceaxxxx2"
    }
  ]
}

acutally

{
  "whiteList": [
    {
      "data_pool_code_id" : 2,
      "data_pool_nft_contract_adress": "panaceaxxxx2"
    }
  ]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In your example,

{
  "whiteList": [
    {
      "data_pool_code_id" : 2,
      "data_pool_nft_contract_adress": "panaceaxxxx2"
    },
    {
      "data_pool_code_id" : 2,
      "data_pool_nft_contract_adress": "panaceaxxxx2"
    }
  ]
}

The last one was duplicated..

Copy link
Contributor

Choose a reason for hiding this comment

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

That's interesting...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are also differences of types in GenesisState. Without gogoproto extension, pointer is added to each type.

Without [(gogoproto.nullable) = false],

type GenesisState struct {
    DataValidators []*DataValidator `protobuf:"bytes,1,rep,name=data_validators,json=dataValidators,proto3" json:"data_validators"`
    NextPoolNumber uint64           `protobuf:"varint,2,opt,name=next_pool_number,json=nextPoolNumber,proto3" json:"next_pool_number,omitempty"`
    Pools          []*Pool          `protobuf:"bytes,3,rep,name=pools,proto3" json:"pools"`
    Params         *Params          `protobuf:"bytes,4,opt,name=params,proto3" json:"params"`
}

With [(gogoproto.nullable) = false],

type GenesisState struct {
    DataValidators []DataValidator `protobuf:"bytes,1,rep,name=data_validators,json=dataValidators,proto3" json:"data_validators"`
    NextPoolNumber uint64          `protobuf:"varint,2,opt,name=next_pool_number,json=nextPoolNumber,proto3" json:"next_pool_number,omitempty"`
    Pools          []Pool          `protobuf:"bytes,3,rep,name=pools,proto3" json:"pools"`
    Params         Params          `protobuf:"bytes,4,opt,name=params,proto3" json:"params"`
}

Copy link
Contributor

Choose a reason for hiding this comment

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

There are also differences of types in GenesisState. Without gogoproto extension, pointer is added to each type.

This behavior is intended. But still, it's weird that the list contains all the same value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

�> Is this issue still being raised? Then, I will take a look at it.

This issue is resolved now, but I don't know what's going on.

This behavior is intended.

Oh I see. I'm curious this part as well. I'll look at gogoproto extension more.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just removed [(gogoproto.nullable) = false] and checked that the exported list in the genesis contains two different elements. As I know, [(gogoproto.nullable) = false] shouldn't violate the fundamental behavior of protobuf marshalling/unmarhalling.

In protobuf, the nullability is a bit tricky to handle because everything is optional by default in protobuf 3. That's why all fields are defined with pointer such as []*DataValidator or *Params. Previously, the protobuf 2 had the required keyword which sets some field not nullable, but that feature was deleted in protobuf 3: protocolbuffers/protobuf#2497.

That's why the gogoproto, which is an extension of protobuf, introduced a new feature nullable. If you really want to make each list element not nullable, then you can use [(gogoproto.nullable) = false] so that gogoproto defines the list as []Element, not []*Element. I also agree with using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

But still, it's weird that the list contains all the same value.

I guess some Go code wasn't implemented correctly at that time.

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, I see. I'm very thankful to you for your �detailed explanation.

I guess some Go code wasn't implemented correctly at that time.

I think it could be, because as I remember It was exported correctly when I implement ExportGenesis() of module params in #294.

Base automatically changed from ft/275/get-params-query to ft/275/wasm-gov-contract April 13, 2022 11:36
Base automatically changed from ft/275/wasm-gov-contract to master April 13, 2022 11:49
x/datapool/keeper/pool.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 left a few comment :)

Comment on lines 12 to 16
repeated DataValidator data_validators = 1 [(gogoproto.nullable) = false];
uint64 next_pool_number = 2;
repeated Pool pools = 3 [(gogoproto.nullable) = false];
Params params = 4 [(gogoproto.nullable) = false];
repeated WhiteList white_list = 5 [(gogoproto.nullable) = false];
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you saying this?
expected:

{
  "whiteList": [
    {
      "data_pool_code_id" : 1,
      "data_pool_nft_contract_adress": "panaceaxxxx1"
    },
    {
      "data_pool_code_id" : 2,
      "data_pool_nft_contract_adress": "panaceaxxxx2"
    }
  ]
}

acutally

{
  "whiteList": [
    {
      "data_pool_code_id" : 2,
      "data_pool_nft_contract_adress": "panaceaxxxx2"
    }
  ]
}

x/datapool/README.md Outdated Show resolved Hide resolved
x/datapool/keeper/pool.go Outdated Show resolved Hide resolved
x/datapool/keeper/pool.go Outdated Show resolved Hide resolved
proto/panacea/datapool/v2/genesis.proto Outdated Show resolved Hide resolved
hansol-medi added 4 commits April 15, 2022 12:21
# Conflicts:
#	x/datapool/client/cli/tx.go
#	x/datapool/client/cli/txPool.go
#	x/datapool/handler.go
#	x/datapool/keeper/pool.go
#	x/datapool/types/errors.go
#	x/datapool/types/keys.go
#	x/datapool/types/pool.go
Copy link

@inchori inchori left a comment

Choose a reason for hiding this comment

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

LGTM 👍

x/datapool/keeper/pool.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.

LGTM 👍

@youngjoon-lee
Copy link
Contributor

youngjoon-lee commented Apr 22, 2022

At

return []types.DataValidator{}, err
, you don't needa return an empty slice which allocates memory. Just return nil, err. The zero value of slice is nil.

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.

lgtm

Copy link

@inchori inchori left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@0xHansLee
Copy link
Contributor Author

you don't needa return an empty slice which allocates memory. Just return nil, err. The zero value of slice is nil.

Thank you @youngjoon-lee !
It is changed in 77df372

@@ -492,3 +484,61 @@ func (k Keeper) GetDataValidationCertificate(ctx sdk.Context, poolID, round uint

return cert, nil
}

func (k Keeper) BuyDataPass(ctx sdk.Context, buyer sdk.AccAddress, poolID, round uint64, payment sdk.Coin) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

We must distribute a DataPass to a seller every time it is sold. So, when this PR is merged, I will add it to distribute to sellers whenever DataPass is sold.

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.

LGTM 👍

@0xHansLee 0xHansLee merged commit d0f76dc into master Apr 25, 2022
@0xHansLee 0xHansLee deleted the ft/279/buy-data-access-nft branch April 25, 2022 02:03
@0xHansLee 0xHansLee linked an issue May 25, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Market Data Marketplace for Web3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement MsgBuyDataAccessNFT transaction
4 participants