Separation of issuer and admin for fungible tokens #805

masihyeganeh · 2024-04-17T09:05:33Z

Description

Reviewers checklist:

Try to write more meaningful comments with clear actions to be taken.
Nit-picking should be unblocking. Focus on core issues.

Authors checklist

Provide a concise and meaningful description
Review the code yourself first, before making the PR.
Annotate your PR in places that require explanation.
Think and try to split the PR to smaller PR if it is big.

This change is

codecov · 2024-04-17T09:58:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.90%. Comparing base (ac6d558) to head (0b18889).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #805      +/-   ##
==========================================
+ Coverage   31.74%   34.90%   +3.16%     
==========================================
  Files         179      181       +2     
  Lines       50608    51805    +1197     
==========================================
+ Hits        16064    18084    +2020     
+ Misses      31374    30213    -1161     
- Partials     3170     3508     +338

Flag	Coverage Δ
coreum	`31.82% <ø> (+0.08%)`	⬆️
coreum-integration-tests-modules	`17.09% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

miladz68

Reviewed 28 of 28 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @masihyeganeh)

integration-tests/modules/wasm_test.go line 1361 at r1 (raw file):

	requireT.NoError(json.Unmarshal(queryOut, &wasmTokenRes))
	wasmTokenRes.Token.Version = expectedToken.Version // test should work with any version
	// TODO(masih): Remove this line, once WASM contract is updated

by wasm contract you mean our rust sdk ?

x/asset/ft/migrations/v3/definitions.go line 25 at r1 (raw file):

		def.Admin = def.Issuer
		keeper.SetDefinition(ctx, issuer, subunit, def)

definition has the issuer field, use that

dzmitryhil

Reviewable status: 20 of 29 files reviewed, 4 unresolved discussions (waiting on @masihyeganeh, @miladz68, @wojtek-coreum, and @ysv)

proto/coreum/asset/ft/v1/tx.proto line 45 at r2 (raw file):

  // TransferAdmin changes admin of a fungible token.
  rpc TransferAdmin(MsgTransferAdmin) returns (EmptyResponse);

Don't we need the admin removal ?
@ysv @miladz68 @wojtek-coreum

x/asset/ft/keeper/migrations.go line 39 at r2 (raw file):

// Migrate3to4 migrates from version 3 to 4.
func (m Migrator) Migrate3to4(ctx sdk.Context) error {

You also need to add upgrade integration test for it, please check (integration-tests/upgrade/upgrade_test.go).
It might require the crust changes as well.

masihyeganeh

Reviewable status: 20 of 29 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)

integration-tests/modules/wasm_test.go line 1361 at r1 (raw file):

Previously, miladz68 (milad) wrote…

by wasm contract you mean our rust sdk ?

Done.

proto/coreum/asset/ft/v1/tx.proto line 45 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Don't we need the admin removal ?
@ysv @miladz68 @wojtek-coreum

A follow up question would be:
Are we going to support having multiple admins for a token or not

x/asset/ft/keeper/migrations.go line 39 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You also need to add upgrade integration test for it, please check (integration-tests/upgrade/upgrade_test.go).
It might require the crust changes as well.

Should I write an upgrade for v4 or v3patch3?

x/asset/ft/migrations/v3/definitions.go line 25 at r1 (raw file):

Previously, miladz68 (milad) wrote…

definition has the issuer field, use that

I'm gonna need the subunit anyways, so it won't do much here

dzmitryhil

Reviewable status: 19 of 29 files reviewed, 4 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

proto/coreum/asset/ft/v1/tx.proto line 45 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

A follow up question would be:
Are we going to support having multiple admins for a token or not

I mean do we need an sdm.Message to remove admin completely?

masihyeganeh

Reviewable status: 19 of 29 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)

proto/coreum/asset/ft/v1/tx.proto line 45 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I mean do we need an sdm.Message to remove admin completely?

I don't think so. This would limit sovereignty of a token indefinitely. If you have a use-case for it, we can discuss it after daily

x/asset/ft/keeper/migrations.go line 39 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

Should I write an upgrade for v4 or v3patch3?

And also I think I need to wait for #809 to merge first

miladz68

Reviewed 9 of 9 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

proto/coreum/asset/ft/v1/tx.proto line 45 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

I don't think so. This would limit sovereignty of a token indefinitely. If you have a use-case for it, we can discuss it after daily

we would like to have that feature probably. But we should discuss it with Reza first.

dzmitryhil

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @masihyeganeh, @wojtek-coreum, and @ysv)

proto/coreum/asset/ft/v1/tx.proto line 45 at r2 (raw file):

Previously, miladz68 (milad) wrote…

we would like to have that feature probably. But we should discuss it with Reza first.

Let's discuss it today.

x/asset/ft/keeper/migrations.go line 39 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

And also I think I need to wait for #809 to merge first

Right.

masihyeganeh

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

proto/coreum/asset/ft/v1/tx.proto line 45 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Let's discuss it today.

It is decided that we need this feature but as an other task

x/asset/ft/keeper/migrations.go line 39 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Right.

Done.

masihyeganeh

Reviewable status: 13 of 32 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)

proto/coreum/asset/ft/v1/tx.proto line 45 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

It is decided that we need this feature but as an other task

Dropping admin functionality added

dzmitryhil

Reviewable status: 13 of 32 files reviewed, 2 unresolved discussions (waiting on @masihyeganeh, @miladz68, @wojtek-coreum, and @ysv)

integration-tests/modules/wasm_test.go line 1361 at r4 (raw file):

	requireT.NoError(json.Unmarshal(queryOut, &wasmTokenRes))
	wasmTokenRes.Token.Version = expectedToken.Version // test should work with any version
	// TODO(masih): Remove this line, once WASM SDK is updated

@keyleu Could you please add a TODO for the WASM SDK update to resolve all Remove this line, once WASM SDK is updated comments.

proto/coreum/asset/ft/v1/tx.proto line 136 at r4 (raw file):

}

message MsgDropAdmin {

You mentioned that it should be in a separate task, but it's here :-)
RemoveAdmin IMO is a better name or ClearAdmin (similar to wasm). @ysv @miladz68 @wojtek-coreum WDYT ?

x/asset/ft/keeper/keeper.go line 983 at r4 (raw file):

	if def.IsAdmin(addr) {
		return sdkerrors.Wrap(cosmoserrors.ErrUnauthorized, "issuer's balance can't be frozen")

Update error text, and check whether there are other errors with issuer.

masihyeganeh

Reviewable status: 13 of 32 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)

proto/coreum/asset/ft/v1/tx.proto line 136 at r4 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You mentioned that it should be in a separate task, but it's here :-)
RemoveAdmin IMO is a better name or ClearAdmin (similar to wasm). @ysv @miladz68 @wojtek-coreum WDYT ?

Yeah, I was going to do that in a separate PR, but since my PRs are open for a while and they have some fundamental changed, I did it here.
I'm open to change the name but it is good enough IMO

x/asset/ft/keeper/keeper.go line 983 at r4 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Update error text, and check whether there are other errors with issuer.

Done.

miladz68

Reviewed 19 of 19 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

proto/coreum/asset/ft/v1/tx.proto line 136 at r4 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

Yeah, I was going to do that in a separate PR, but since my PRs are open for a while and they have some fundamental changed, I did it here.
I'm open to change the name but it is good enough IMO

ClearAdmin is better.

masihyeganeh

Reviewable status: 11 of 32 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)

proto/coreum/asset/ft/v1/tx.proto line 136 at r4 (raw file):

Previously, miladz68 (milad) wrote…

ClearAdmin is better.

Done.

keyleu

Reviewable status: 11 of 32 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)

integration-tests/modules/wasm_test.go line 1361 at r4 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

@keyleu Could you please add a TODO for the WASM SDK update to resolve all Remove this line, once WASM SDK is updated comments.

We have a task for this already.

dzmitryhil

Reviewed 9 of 9 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)

miladz68

Reviewed 9 of 9 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)

masihyeganeh force-pushed the masih/separate-issuer-and-admin-role-in-ft branch from 902dc01 to 3b40565 Compare April 17, 2024 11:02

masihyeganeh changed the title ~~Draft implementation of separation of issuer and admin for fungible t…~~ [WIP] Separation of issuer and admin for fungible t… Apr 18, 2024

masihyeganeh changed the title ~~[WIP] Separation of issuer and admin for fungible t…~~ [WIP] Separation of issuer and admin for fungible tokens Apr 18, 2024

miladz68 reviewed Apr 18, 2024

View reviewed changes

masihyeganeh force-pushed the masih/separate-issuer-and-admin-role-in-ft branch from 90405fd to 45e7f1e Compare April 19, 2024 16:29

masihyeganeh changed the title ~~[WIP] Separation of issuer and admin for fungible tokens~~ Separation of issuer and admin for fungible tokens Apr 22, 2024

masihyeganeh marked this pull request as ready for review April 22, 2024 08:48

masihyeganeh requested a review from a team as a code owner April 22, 2024 08:48

masihyeganeh requested review from dzmitryhil, miladz68, ysv and wojtek-coreum and removed request for a team April 22, 2024 08:48

dzmitryhil requested changes Apr 22, 2024

View reviewed changes

masihyeganeh commented Apr 25, 2024

View reviewed changes

dzmitryhil requested changes Apr 26, 2024

View reviewed changes

masihyeganeh commented Apr 26, 2024

View reviewed changes

miladz68 previously approved these changes Apr 26, 2024

View reviewed changes

dzmitryhil requested changes Apr 26, 2024

View reviewed changes

masihyeganeh commented Apr 26, 2024

View reviewed changes

masihyeganeh dismissed miladz68’s stale review via a18f563 April 29, 2024 08:19

masihyeganeh commented Apr 29, 2024

View reviewed changes

dzmitryhil requested changes Apr 29, 2024

View reviewed changes

masihyeganeh commented Apr 29, 2024

View reviewed changes

miladz68 previously approved these changes Apr 29, 2024

View reviewed changes

masihyeganeh dismissed miladz68’s stale review via cc7278c April 29, 2024 11:08

masihyeganeh force-pushed the masih/separate-issuer-and-admin-role-in-ft branch from 7359b67 to cc7278c Compare April 29, 2024 11:08

masihyeganeh commented Apr 29, 2024

View reviewed changes

keyleu reviewed Apr 29, 2024

View reviewed changes

masihyeganeh added 22 commits May 3, 2024 10:21

Fix integration tests round 2

87f579e

Removing checks that don't belong to ValidateBasic

4b32326

Fix integration tests round 3

bff9120

Fix integration tests round 4

a5d04cf

Fix integration tests round 5

89254a1

Fix integration tests round 6

31759ed

Fix integration tests round 7

702122a

Fix integration tests round 8

4a6a590

Fix integration tests round 9

59d2379

Fix integration tests round 10

ee05a42

Fix integration tests

6de7a26

Fix all integration tests

c8bb2c1

Fix comment

b715aec

Add upgrade test

75610f6

Add drop admin functionality

cea8a83

Fix error messages

e5e50a5

Fix conflicts

63e64ec

Rename DropAdmin to ClearAdmin

789e1eb

Add tests for clawback after transferring admin

078f885

Fix conflicts

9c49448

Fix protos

44f7da8

Fix commission rate

8d73459

masihyeganeh dismissed stale reviews from dzmitryhil and miladz68 via 8d73459 May 3, 2024 06:51

masihyeganeh force-pushed the masih/separate-issuer-and-admin-role-in-ft branch from 87073e8 to 8d73459 Compare May 3, 2024 06:51

Fix TestDeterministicGas_DeterministicMessages

0b18889

dzmitryhil approved these changes May 3, 2024

View reviewed changes

miladz68 approved these changes May 3, 2024

View reviewed changes

masihyeganeh merged commit d93b3be into master May 3, 2024
10 checks passed

masihyeganeh deleted the masih/separate-issuer-and-admin-role-in-ft branch May 3, 2024 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separation of issuer and admin for fungible tokens #805

Separation of issuer and admin for fungible tokens #805

masihyeganeh commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading

miladz68 left a comment

dzmitryhil left a comment

masihyeganeh left a comment

dzmitryhil left a comment

masihyeganeh left a comment

miladz68 left a comment

dzmitryhil left a comment

masihyeganeh left a comment

masihyeganeh left a comment

dzmitryhil left a comment

masihyeganeh left a comment

miladz68 left a comment

masihyeganeh left a comment

keyleu left a comment

dzmitryhil left a comment

miladz68 left a comment

Separation of issuer and admin for fungible tokens #805

Separation of issuer and admin for fungible tokens #805

Conversation

masihyeganeh commented Apr 17, 2024 • edited Loading

Description

Reviewers checklist:

Authors checklist

codecov bot commented Apr 17, 2024 • edited Loading

Codecov Report

miladz68 left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

keyleu left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

masihyeganeh commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading