fix: refining ext and test contract #35

shiki-tak · 2020-11-26T07:16:17Z

Description

Correction of cosmwasm due to the effect of refactoring the encoder in the token, collection module in the PR of https:/line/link-modules/pull/60

Motivation and context

How has this been tested?

Checklist:

I followed the contributing guidelines.
I have updated the documentation accordingly.
I have added tests to cover my changes.

whylee259 · 2020-12-10T02:09:47Z

packages/ext/src/collection.rs

@@ -13,20 +13,15 @@ pub struct Collection {
 }

 #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
-pub struct NonFungibleToken {
+pub struct Token {


As the original response was separated into two concrete types, I think it would be better to separate the client-side as well.

https:/line/link-modules/blob/a0eebafe46b1ac0a46600a24bb3859f132c165eb/x/collection/internal/types/codec.go#L45-L46

I agree. If they have common functions, these should be implemented as traits.

The reason why I grouped them together in a structure called "Token" is because both FT and NFT types are returned to the client-side in query_tokens.
https:/line/link-modules/blob/20e848dbda3ce486059e40d7d63bbf6c4319f898/x/collection/client/internal/types/retriever.go#L152

If I have separated two concrete types, I cannot parse them and get an error.

The query_token is not returned as a Token, but as a Response type.
https:/line/cosmwasm/pull/35/files#diff-11044ca7e42123842acfa85d0bfef49ca79f44729704a9fc929e412d1ad9b104R161

Therefore, The key of Response type can be used to determine the concrete types (FT or NFT).
https:/line/cosmwasm/blob/develop/packages/ext/src/query.rs#L69-L70

Ref.

https:/line/link-modules/blob/20e848dbda3ce486059e40d7d63bbf6c4319f898/x/collection/internal/types/token.go#L9-L31

https:/line/link-modules/blob/20e848dbda3ce486059e40d7d63bbf6c4319f898/x/collection/client/internal/types/retriever.go#L152-L163

If you want to use direct sum types in Rust, you can use an enum to treat FT and NFT as common Token types, as shown below.

pub struct FT{ ... } impl FT { fn A(){ ... }, fn B(){ ... }, ... } pub struct NFT{ ... } impl NFT { fn A(){ ... }, fn C(){ ... }, ... } pub enum Token { Fungible(FT), NonFungible(NFT), } impl Token { fn A(){ ... }, ... } // common behaviour between FT and NFT let tokens = vec![Token::Fungible(FT{...}), Token::NonFungible(NFT{...})]; for token in tokens.iter() { token.A(); match token { Token::Fungible(ft) => { ft.B(); ... }, Token::NonFungible(nft) => { nft.C(); ... }, } }

However, you probably need to make sure whether the serialized format of Token will be in your intended.

whylee259 · 2020-12-10T02:19:21Z

packages/ext/src/querier_collection.rs

+ proxy: HumanAddr,
+ contract_id: String,


Suggested change

proxy: HumanAddr,

contract_id: String,

contract_id: String,

proxy: HumanAddr,

minor suggestion

whylee259 · 2020-12-10T02:36:54Z

packages/ext/src/querier_collection.rs

@@ -53,7 +54,6 @@ pub enum CollectionQuery {
 QueryTotalParam {
 contract_id: String,
 token_id: String,
- target: Target,
 },
 QueryPermParam {


I think this param is for QueryIsApproved (https:/line/link-modules/blob/develop/x/collection/internal/querier/querier.go#L55), is it right?
But the naming is confused with QueryPerms (https:/line/link-modules/blob/develop/x/collection/internal/querier/querier.go#L25).

I think this param is for QueryIsApproved

Confused with QueryApprovedParam?

But the naming is confused with QueryPerms

QueryXxxx sounds like a query, and ParamXxxx sounds like a parameter.
In this case, replacing QueryXxxxParam -> QueryXxxx or replacing QueryXxxxParam -> ParamXxxx are better. I prefer the former. (... But it should be another issue

There are two types of permission,

account permission (mint, burn ...)

proxy permission (approver)

In the chain, the first case is being named perm and the second case is being named approve.
However, this code line is used for proxy permission but named perm

But this comment is not related to changes in the PR.
We can discuss it on another issue.

QueryPermParam is a parameter to query account permission.

rename QueryXxxxParam to XxxxParam

loloicci

I think this patch is overkill for refactoring.
This PR changes the interfaces (messages), adding some features, and fixing some bugs.

I want to know what is your intention for this patch in more detail by reading commit messages. It is hard to review this PR with these information for me...

…ry_total, query_mint and query_burn in token module

…ry_mint and query_burn in collection module

shiki-tak · 2020-12-15T10:48:18Z

I think this patch is overkill for refactoring.
This PR changes the interfaces (messages), adding some features, and fixing some bugs.

I want to know what is your intention for this patch in more detail by reading commit messages. It is hard to review this PR with these information for me...

sorry, I've split the commit. PTAL.
If you need more information than this, please ask specific questions.

loloicci · 2020-12-16T03:33:58Z

packages/ext/src/collection.rs

@@ -13,20 +13,15 @@ pub struct Collection {
 }

 #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
-pub struct NonFungibleToken {
+pub struct Token {


I agree. If they have common functions, these should be implemented as traits.

loloicci · 2020-12-16T03:41:01Z

packages/ext/src/querier_token.rs

@@ -84,15 +85,38 @@ impl<'a, Q: Querier> LinkTokenQuerier<'a, Q> {
 Ok(res)
 }

- pub fn query_supply(&self, contract_id: String, target: Target) -> StdResult<Uint128> {
+ pub fn query_supply(&self, contract_id: String) -> StdResult<Uint128> {


refactor: remove target param from query_supply and split it into que…

It looks like a changing of spec. Isn't it?

OK, then, I think this commit message should not say "refactor:".

loloicci · 2020-12-16T03:45:46Z

packages/ext/src/querier_collection.rs

@@ -53,7 +54,6 @@ pub enum CollectionQuery {
 QueryTotalParam {
 contract_id: String,
 token_id: String,
- target: Target,
 },
 QueryPermParam {


I think this param is for QueryIsApproved

Confused with QueryApprovedParam?

But the naming is confused with QueryPerms

QueryXxxx sounds like a query, and ParamXxxx sounds like a parameter.
In this case, replacing QueryXxxxParam -> QueryXxxx or replacing QueryXxxxParam -> ParamXxxx are better. I prefer the former. (... But it should be another issue

loloicci · 2020-12-24T08:40:07Z

packages/ext/src/collection.rs

+ meta,
+ owner: HumanAddr::from(owner),
+ }),
+ _ => panic!("unexpected token type"),


This function should not panic, because the returning type is Result.
Instead, it should return Err(foo).

shiki-tak · 2020-12-24T08:56:44Z

PTAL

loloicci

Great! But, I think we need some unit tests for serializing/deserializing defined in packages/ext/src/collection.rs. Especially, it is needed for a list of FT/NFT.

loloicci

LGTM!

shiki-tak · 2020-12-25T03:01:09Z

PTAL @whylee259

whylee259 · 2020-12-28T15:38:52Z

packages/ext/src/collection.rs

+#[derive(Serialize, Clone, Debug, PartialEq, JsonSchema)]
+#[serde(untagged)]
+pub enum Token {
+ FT(FungibleToken),
+ NFT(NonFungibleToken),
+}
+


I wonder why this uses a manually implemented deserializer rather than use derive as follows.

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] #[serde(untagged)] pub enum Token { FT(FungibleToken), NFT(NonFungibleToken), }

Additionally, using Adjacently tag, seems to be able to reduce the depth of response struct.

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] #[serde(tag = "type", content = "value")] pub enum Token { #[serde(rename = "collection/FT")] FT(FungibleToken), #[serde(rename = "collection/NFT")] NFT(NonFungibleToken), } let res = serde_json::from_str::<Vec<Token>>(json);

but we can discuss reducing depth on another issue.

Let's do this at #50.

whylee259 · 2020-12-28T16:18:25Z

If only one curiosity I commented on is resolved,
LGTM! 👍

* feat: add query approved function for collection module * fix: fix LinkQueryWrapper type * fix: rename query_approved to query_is_approved * fix: bug fix query_is_approved return value * refactor: unify NFT and FT types into one * refactor: change the arguments of modify function * refactor: rename GetNft to GetNftCount * chore: add snake_case * refactor: remove target param from query_supply and split it into query_total, query_mint and query_burn in token module * refactor: remove target param from query_supply and split it into query_mint and query_burn in collection module * fix: update schema json * fix: separate token into FT and NFT and implement deserialize * fix: swap the order of parameters * fix: rename query param * fix: fix lint error * fix: change return error with match statement * chore: add unit test for deserialize * fix: fix ci error

shiki-tak added good first issue Good for newcomers VM WIP and removed good first issue Good for newcomers labels Dec 1, 2020

shiki-tak self-assigned this Dec 3, 2020

shiki-tak requested review from whylee259, loloicci and brew0722 December 3, 2020 10:25

shiki-tak removed the WIP label Dec 3, 2020

shiki-tak changed the title ~~fix: tester contract~~ fix: refactoring ext and test contract Dec 3, 2020

whylee259 reviewed Dec 10, 2020

View reviewed changes

loloicci suggested changes Dec 15, 2020

View reviewed changes

shiki-tak added 10 commits December 15, 2020 19:06

feat: add query approved function for collection module

58909fa

fix: fix LinkQueryWrapper type

4ee04a6

fix: rename query_approved to query_is_approved

da450cb

fix: bug fix query_is_approved return value

8d6bb70

refactor: unify NFT and FT types into one

965723c

refactor: change the arguments of modify function

437d961

refactor: rename GetNft to GetNftCount

58a351f

chore: add snake_case

60e0b13

refactor: remove target param from query_supply and split it into que…

048926e

…ry_total, query_mint and query_burn in token module

refactor: remove target param from query_supply and split it into que…

5fceebf

…ry_mint and query_burn in collection module

fix: update schema json

765e93b

loloicci mentioned this pull request Dec 15, 2020

Merging 0.12 #39

Merged

3 tasks

shiki-tak changed the title ~~fix: refactoring ext and test contract~~ fix: refining ext and test contract Dec 15, 2020

loloicci reviewed Dec 16, 2020

View reviewed changes

shiki-tak added 3 commits December 24, 2020 14:41

fix: separate token into FT and NFT and implement deserialize

f4a0140

fix: swap the order of parameters

29e6b81

fix: rename query param

b840ea9

loloicci reviewed Dec 24, 2020

View reviewed changes

shiki-tak added 2 commits December 24, 2020 17:51

fix: fix lint error

776b5b2

fix: change return error with match statement

0ef4571

loloicci self-requested a review December 24, 2020 10:13

loloicci reviewed Dec 24, 2020

View reviewed changes

loloicci self-requested a review December 24, 2020 10:17

shiki-tak added 2 commits December 24, 2020 20:15

chore: add unit test for deserialize

39c48e2

fix: fix ci error

a4a54a9

loloicci approved these changes Dec 25, 2020

View reviewed changes

whylee259 reviewed Dec 28, 2020

View reviewed changes

whylee259 mentioned this pull request Dec 29, 2020

Remove Response using Adjacently tag #50

Closed

whylee259 approved these changes Dec 29, 2020

View reviewed changes

shiki-tak merged commit 5a5c784 into Finschia:develop Dec 29, 2020

shiki-tak mentioned this pull request Jan 5, 2021

rename QueryXxxxParam to XxxxParam #55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: refining ext and test contract #35

fix: refining ext and test contract #35

shiki-tak commented Nov 26, 2020 •

edited

Loading

whylee259 Dec 10, 2020

loloicci Dec 16, 2020

shiki-tak Dec 16, 2020

tnasu Dec 18, 2020

torao Dec 18, 2020 •

edited

Loading

whylee259 Dec 10, 2020

shiki-tak Dec 24, 2020

whylee259 Dec 10, 2020

loloicci Dec 16, 2020 •

edited

Loading

whylee259 Dec 18, 2020

shiki-tak Dec 21, 2020

shiki-tak Dec 24, 2020

loloicci left a comment

shiki-tak commented Dec 15, 2020

loloicci Dec 16, 2020

loloicci Dec 16, 2020

shiki-tak Dec 16, 2020

loloicci Dec 17, 2020

loloicci Dec 16, 2020 •

edited

Loading

loloicci Dec 24, 2020

shiki-tak commented Dec 24, 2020

loloicci left a comment

loloicci left a comment

shiki-tak commented Dec 25, 2020

whylee259 Dec 28, 2020 •

edited

Loading

whylee259 Dec 29, 2020

whylee259 commented Dec 28, 2020

fix: refining ext and test contract #35

fix: refining ext and test contract #35

Conversation

shiki-tak commented Nov 26, 2020 • edited Loading

Description

Motivation and context

How has this been tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torao Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loloicci Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loloicci left a comment

Choose a reason for hiding this comment

shiki-tak commented Dec 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loloicci Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiki-tak commented Dec 24, 2020

loloicci left a comment

Choose a reason for hiding this comment

loloicci left a comment

Choose a reason for hiding this comment

shiki-tak commented Dec 25, 2020

whylee259 Dec 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whylee259 commented Dec 28, 2020

shiki-tak commented Nov 26, 2020 •

edited

Loading

torao Dec 18, 2020 •

edited

Loading

loloicci Dec 16, 2020 •

edited

Loading

loloicci Dec 16, 2020 •

edited

Loading

whylee259 Dec 28, 2020 •

edited

Loading