Allow setting manager, reserve, freeze, and clawback at goal asset create #3369

fionnachan · 2022-01-05T07:41:35Z

Summary

Allow setting manager, reserve, freeze, and clawback at goal asset create

Test Plan

Did not test it.
I built the binaries, ran ${GOPATH}/bin/goal node start -d ~/.algorand, used goal to create a new wallet, but I don't know how to fund the wallet to continue :(

Closes #3361

…eate

jannotti · 2022-01-05T18:05:19Z

Thank you! This looks good by eye, we just need some simple tests. You could add them to test/scripts/e2e_subs/asset-misc.sh

You could do another asset create, similar to line 24, and then confirm that the various addresses were set appropriately with some grep lines similar to line 26.

To run just that test, you should be able to do ./test/scripts/e2e_client_runner.py ./test/scripts/e2e_subs/asset-misc.sh from the top of your repo. That will take care of setting up a temporary test network, funding a wallet, and then invoking that script so that it can do the things inside.

fionnachan · 2022-01-06T15:15:05Z

Added a test for --no-manager --no-freezer --no-clawback and another test for --creator "${ACCOUNT}" --manager "${ACCOUNTB}". Let me know if you want tests for --freezer / --clawback separately.

I removed the --no-reserve flag because even when the reserve is set to empty explicitly, the reserve will be the creator anyway.

jannotti · 2022-01-06T17:23:28Z

I removed the --no-reserve flag because even when the reserve is set to empty explicitly, the reserve will be the creator anyway.

This is weird. I don't think this should happen, I can't find any code in goal or algod that would do this on purpose, so I'd like to figure out if there's a bug here. What exactly did you see?

jannotti

Once we figure out this reserve address thing, it looks good!

test/scripts/e2e_subs/asset-misc.sh

jannotti · 2022-01-06T17:16:56Z

test/scripts/e2e_subs/asset-misc.sh

+# create asset with a manager that is different from the creator
+${gcmd} asset create --creator "${ACCOUNT}" --manager "${ACCOUNTB}" --name "${ASSET_NAME}" --unitname dma --total 1000000000000 --asseturl "${ASSET_URL}"


I'd prefer that this sets all of the different address types, not just manager, and that it use a different address for each. That way we are testing against simple cut and paste bugs where, for example, using --freezer X actually sets the reserve address.

jannotti · 2022-01-06T17:17:48Z

test/scripts/e2e_subs/asset-misc.sh

+if [ "$DMA_MANAGER_ADDRESS" = "$ACCOUNTB" ] \
+    && [ "$DMA_RESERVE_ADDRESS" = "$ACCOUNT" ] \
+    && [ "$DMA_FREEZE_ADDRESS" = "$ACCOUNT" ] \
+    && [ "$DMA_CLAWBACK_ADDRESS" = "$ACCOUNT" ]; then


This is where I'd like to see different equality checks for each address.

jannotti · 2022-01-06T17:29:06Z

cmd/goal/asset.go

+		if cmd.Flags().Changed("manager") {
+			assetManager = accountList.getAddressByName(assetManager)
+			manager = assetManager
+		}
+
+		if cmd.Flags().Changed("no-manager") {


I suppose it would be nice to have a check to see if both "manager" is set, and "no-manager" is true, and if so, error out. (Same for others too.)

Done. For the flag checks, I used cmd.Flags().Changed("manager") && cmd.Flags().Changed("no-manager") instead of cmd.Flags().Changed("manager") && assetNoManager because it looks nicer with two flags on the same line.... 😬

You can also use the variable for the manager flag directly, so you could do something as simple as assetManager != "" && assetNoManager without any use of cmd.Flags().Changed because they both have defaults set, so you would report the error if they both differ from their default.

I prefer to use the variables directly and only use Changed if I need to distinguish between when the switch gets its default value because it was not given on the command line, vs being given on the command-line, but being the same as the default. Here, the distinction does not matter.

jannotti · 2022-01-06T17:41:29Z

cmd/goal/asset.go

+			reserve = assetReserve
+		}
+
+		if cmd.Flags().Changed("no-reserve") {


A slightly simpler way to use boolean flags is to set the default to false (which you've already done), and then you don't need to use Changed(), you can just use the variable directly. See for example, how assetFrozen is used.

jannotti · 2022-01-06T17:46:08Z

Oh, sorry, I missed that. Of course you need it.

…

On Thu, Jan 6, 2022 at 12:44 PM Fionna Chan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/scripts/e2e_subs/asset-misc.sh <#3369 (comment)>: > @@ -47,4 +47,44 @@ else exit 1 fi +# create asset with no manager, no freezer, and no clawback +${gcmd} asset create --creator "${ACCOUNT}" --no-manager --no-freezer --no-clawback --name "${ASSET_NAME}" --unitname iamisc --total 1000000000000 --asseturl "${ASSET_URL}" + +IMMUTABLE_ASSET_ID=$(${gcmd} asset info --creator $ACCOUNT --unitname iamisc|grep 'Asset ID'|awk '{ print $3 }') It was used for goal asset info to get the addresses. Is there a better way to get them? :/ — Reply to this email directly, view it on GitHub <#3369 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADL7T5C7JRHKMSNOYSMMD3UUXIHZANCNFSM5LJHUACA> . You are receiving this because you commented.Message ID: ***@***.***>

fionnachan · 2022-01-06T17:46:32Z

I am seeing this with --no-reserve, which I didn't expect at all either before writing the test....

+ goal -w AF4A62B9687CF6BBAB8427229BE8F2D9 asset create --creator UOTRSRYEAHIPBIAOQA6HW3OI6FCRLXABNPPANGZ7WJJN2HTTTKKJAH3OII --no-manager --no-reserve --no-freezer --no-clawback --name 'Birlot : décollage vs. ࠶🦪' --unitname iamisc --total 1000000000000 --asseturl /ipfs/QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco/wiki/Verifiable_random_function.html
Issued transaction from account UOTRSRYEAHIPBIAOQA6HW3OI6FCRLXABNPPANGZ7WJJN2HTTTKKJAH3OII, txid GCM66CSDDU36KB224LVIC7WU5LWNTYXLLSVZO7UYTULJQ6DIIVUQ (fee 1000)
Transaction GCM66CSDDU36KB224LVIC7WU5LWNTYXLLSVZO7UYTULJQ6DIIVUQ still pending as of round 20
Transaction GCM66CSDDU36KB224LVIC7WU5LWNTYXLLSVZO7UYTULJQ6DIIVUQ still pending as of round 21
Transaction GCM66CSDDU36KB224LVIC7WU5LWNTYXLLSVZO7UYTULJQ6DIIVUQ committed in round 22
Created asset with asset index 11
++ goal -w AF4A62B9687CF6BBAB8427229BE8F2D9 asset info --creator UOTRSRYEAHIPBIAOQA6HW3OI6FCRLXABNPPANGZ7WJJN2HTTTKKJAH3OII --unitname iamisc
++ grep 'Asset ID'
++ awk '{ print $3 }'
+ IMMUTABLE_ASSET_ID=11
++ goal -w AF4A62B9687CF6BBAB8427229BE8F2D9 asset info --assetid 11
++ grep 'Manager address'
++ awk '{ print $3 }'
+ MANAGER_ADDRESS=
++ goal -w AF4A62B9687CF6BBAB8427229BE8F2D9 asset info --assetid 11
++ grep 'Reserve address'
++ awk '{ print $3 }'
+ RESERVE_ADDRESS=UOTRSRYEAHIPBIAOQA6HW3OI6FCRLXABNPPANGZ7WJJN2HTTTKKJAH3OII
++ goal -w AF4A62B9687CF6BBAB8427229BE8F2D9 asset info --assetid 11
++ grep 'Freeze address'
++ awk '{ print $3 }'
+ FREEZE_ADDRESS=
++ goal -w AF4A62B9687CF6BBAB8427229BE8F2D9 asset info --assetid 11
++ grep 'Clawback address'
++ awk '{ print $3 }'
+ CLAWBACK_ADDRESS=
+ '[' '' = '' ']'
+ '[' UOTRSRYEAHIPBIAOQA6HW3OI6FCRLXABNPPANGZ7WJJN2HTTTKKJAH3OII = '' ']'
+ date '+asset-misc immutable asset info error %Y%m%d_%H%M%S'
asset-misc immutable asset info error 20220106_174132
+ exit 1
20220106_174132 :460 ERRORS after 93.727515 seconds: '/workspaces/go-algorand/test/scripts/e2e_subs/asset-misc.sh failed'
20220106_174132 :463 statuses-json: [{"script": "/workspaces/go-algorand/test/scripts/e2e_subs/asset-misc.sh", "ok": false, "seconds": 88.35015201568604}]
20220106_174132 :329 stop network in /tmp/tmp4nbvmh2a/net
20220106_174133 :338 stop kmd in /tmp/tmp4nbvmh2a/net/Node
20220106_174133 :351 delete network in /tmp/tmp4nbvmh2a/net

fionnachan · 2022-01-06T17:52:36Z

oh look....

go-algorand/cmd/goal/asset.go

Lines 611 to 615 in 52a1a2c

    
           reserveEmpty := false 
        
           if params.ReserveAddr == "" { 
        
           	reserveEmpty = true 
        
           	params.ReserveAddr = params.Creator 
        
           }

go-algorand/cmd/goal/asset.go

Lines 634 to 638 in 52a1a2c

    
           if reserveEmpty { 
        
           	fmt.Printf("Reserve address:  %s (Empty. Defaulting to creator)\n", params.ReserveAddr) 
        
           } else { 
        
           	fmt.Printf("Reserve address:  %s\n", params.ReserveAddr) 
        
           }

jannotti · 2022-01-06T18:03:44Z

oh look....

go-algorand/cmd/goal/asset.go

Lines 611 to 615 in 52a1a2c

reserveEmpty := false

if params.ReserveAddr == "" {

reserveEmpty = true

params.ReserveAddr = params.Creator

}

go-algorand/cmd/goal/asset.go

Lines 634 to 638 in 52a1a2c

if reserveEmpty {

fmt.Printf("Reserve address: %s (Empty. Defaulting to creator)\n", params.ReserveAddr)

} else {

fmt.Printf("Reserve address: %s\n", params.ReserveAddr)

}

Ok, good find. This is only what goal reports and doesn't describe the actual value on-chain. To me, that means you should still support --no-reserve which sets the value on chain to the zero value, but it is very hard to test with goal since goal lies about the value.

jannotti · 2022-01-06T18:18:41Z

Oh, for testing the --no-reserve, you can actually check if the goal output contains Empty. Defaulting to creator

jannotti

This looks good to me, pending CI checks. Thanks again!

The base branch was changed.

algojack · 2022-01-07T22:20:45Z

merging this to a temp branch to kick off CI correctly. Thanks again for your contribution.

jannotti · 2022-01-10T17:08:28Z

This was merged as #3398 because we had a little CI hiccup. Thanks again.

fionnachan · 2022-01-10T17:44:53Z

You're welcome~

Allow setting manager, reserve, freeze, and clawback at goal asset cr…

6e884e9

…eate

fionnachan added 2 commits January 6, 2022 23:10

Add e2e tests and remove --no-reserve flag from goal asset create

94a2678

Remove assetNoReserve bool from goal/asset.go

0fdf841

jannotti reviewed Jan 6, 2022

View reviewed changes

fionnachan added 4 commits January 7, 2022 12:12

Add back no-reserve flag and throw error for mutually exclusive flags

fdeb682

Add more tests for goal asset create flags

da6229a

Fix comment in asset-misc e2e test file

f0231a0

Use variables instead of Changed() for double flag checks

989cc16

jannotti previously approved these changes Jan 7, 2022

View reviewed changes

algojack changed the base branch from master to temp-branch January 7, 2022 21:20

algojack changed the base branch from temp-branch to master January 7, 2022 21:33

algojack changed the base branch from master to temp-branch January 7, 2022 22:20

algojack merged commit 0984eff into algorand:temp-branch Jan 7, 2022

algojack mentioned this pull request Jan 7, 2022

Remaking a PR for CI #3398

Merged

cusma mentioned this pull request Jan 9, 2022

Allowing to specify ASA's management addresses using goal asset create #1705

Closed

fionnachan deleted the goal-asset-create-support-config branch January 10, 2022 17:46

onetechnical mentioned this pull request Jan 18, 2022

go-algorand 3.3.0-beta #3423

Merged

Algo-devops-service mentioned this pull request Jan 20, 2022

Update reference to arm32 build for 3.3.0 release #3461

Merged

Priegle mentioned this pull request Jan 26, 2022

goal asset create - support manager, reserve, freeze, and clawback #3361

Closed

onetechnical mentioned this pull request Mar 1, 2022

go-algorand 3.4.2-stable #3700

Merged

		# create asset with a manager that is different from the creator
		${gcmd} asset create --creator "${ACCOUNT}" --manager "${ACCOUNTB}" --name "${ASSET_NAME}" --unitname dma --total 1000000000000 --asseturl "${ASSET_URL}"

Allow setting manager, reserve, freeze, and clawback at goal asset create #3369

Allow setting manager, reserve, freeze, and clawback at goal asset create #3369

Uh oh!

Conversation

fionnachan commented Jan 5, 2022

Summary

Test Plan

Uh oh!

jannotti commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fionnachan commented Jan 6, 2022

Uh oh!

jannotti commented Jan 6, 2022

Uh oh!

jannotti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jannotti Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

jannotti Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

jannotti Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fionnachan Jan 7, 2022

Choose a reason for hiding this comment

Uh oh!

jannotti Jan 7, 2022

Choose a reason for hiding this comment

Uh oh!

jannotti Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannotti commented Jan 6, 2022 via email

Uh oh!

fionnachan commented Jan 6, 2022

Uh oh!

fionnachan commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jannotti commented Jan 6, 2022

Uh oh!

jannotti commented Jan 6, 2022

Uh oh!

jannotti left a comment

Choose a reason for hiding this comment

Uh oh!

algojack commented Jan 7, 2022

Uh oh!

jannotti commented Jan 10, 2022

Uh oh!

fionnachan commented Jan 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jannotti commented Jan 5, 2022 •

edited

Loading

jannotti Jan 6, 2022 •

edited

Loading

jannotti Jan 6, 2022 •

edited

Loading

fionnachan commented Jan 6, 2022 •

edited

Loading