Authz redesign: CLI implementation #1079

pinosu · 2022-11-07T13:27:02Z

Follow up on #1077

codecov · 2022-11-07T13:42:18Z

Codecov Report

Merging #1079 (0164d85) into main (ef9a84d) will decrease coverage by 0.54%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1079      +/-   ##
==========================================
- Coverage   60.33%   59.79%   -0.55%     
==========================================
  Files          54       54              
  Lines        7211     7273      +62     
==========================================
- Hits         4351     4349       -2     
- Misses       2549     2612      +63     
- Partials      311      312       +1

Impacted Files	Coverage Δ
x/wasm/client/cli/tx.go	`22.94% <0.00%> (-5.12%)`	⬇️
x/wasm/keeper/keeper.go	`87.40% <0.00%> (-0.32%)`	⬇️

alpe

Great start. The CLI will be hard to come with a good UX. I have added some notes to help. My gut feeling is that more concrete parameter may be better to guide the user than overloading max-calls and amount for the different cases. But other opinions are welcome, too.

alpe · 2022-11-08T07:56:55Z

x/wasm/client/cli/tx.go

+
+			var filter types.ContractAuthzFilterX
+			switch len(msgs) {
+			case 0:


I am a bit worried that this becomes the default without giving notice to people. What do you think about an extra parameter that must be set for this option? Something like: --allow-all-messages-wildcard

alpe · 2022-11-08T08:00:53Z

x/wasm/client/cli/tx.go

+					return fmt.Errorf("max funds: %s", err)
+				}
+				limit = types.NewMaxFundsLimit(maxFunds)
+			case maxCalls != 0:


This option does not allow any token transfers. It would be nice if we can be explicit in the cli like --max-calls=1 --no-token-transfer. WDYT?

alpe · 2022-11-08T08:01:34Z

x/wasm/client/cli/tx.go

+			case maxFundsStr != "":
+				maxFunds, err := sdk.ParseCoinsNormalized(maxFundsStr)
+				if err != nil {
+					return fmt.Errorf("max funds: %s", err)


alpe · 2022-11-08T08:04:16Z

x/wasm/client/cli/tx.go

+				filter = types.NewAllowAllMessagesFilter()
+			default:
+				filter = types.NewAcceptedMessageKeysFilter(msgs...)
+			}


The third option is to set the concrete json message(s) (byte equal).

alpe · 2022-11-08T08:04:48Z

x/wasm/client/cli/tx.go

+			}
+
+			var authorization authz.Authorization
+			switch args[1] {


👍 this works

x/wasm/client/cli/tx.go

alpe · 2022-11-08T08:11:19Z

tests/e2e/setup.go

-	coordinator   *ibctesting.Coordinator
-	providerChain *ibctesting.TestChain
-	consumerChain *ibctesting.TestChain
+	// coordinator   *ibctesting.Coordinator


Sorry, I have removed this file on a local branch already. These changes will cause merge conflicts for you when I push.

alpe

Good progress. I added some notes to unblock you. I will do a deeper review tomorrow.

alpe · 2022-11-08T20:53:06Z

x/wasm/client/cli/tx.go

@@ -379,9 +385,9 @@ func parseExecuteArgs(contractAddr string, execMsg string, sender sdk.AccAddress

 func GrantAuthorizationCmd() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:   "grant [grantee] [contract_addr_bech32] --allow-msgs [msg1,msg2,...]",
+		Use:   "grant [grantee] [authorization_type=\"execution\"|\"migration\"] [contract_addr_bech32] --allow-raw-msgs `[msg1,msg2,...]` --allow-msg-keys [key1,key2,...]",


Let's make authorization_type message type. It it the wasmd tx message that can be executed

The ' in "`[msg1,msg2,...]`" were confusing. it is not a json array content but a list of strings

How about adding --allow-all-messages here to have the filters complete? I think some full examples in the Long description would be very useful. The filter + limits model needs to be explained a bit for people to configure this.

alpe · 2022-11-08T20:57:27Z

x/wasm/client/cli/tx.go

 			if err != nil {
 				return err
 			}

-			once, err := cmd.Flags().GetBool(flagRunOnce)
+			rawMsgsStr, err := cmd.Flags().GetString(flagAllowedRawMsgs)


This should be GetStringSlice as well.

alpe · 2022-11-08T20:58:30Z

x/wasm/client/cli/tx.go

-	cmd.Flags().StringSlice(flagAllowedMsgs, []string{}, "Allowed msgs")
-	cmd.Flags().Bool(flagRunOnce, false, "Allow to execute only once")
+	cmd.Flags().StringSlice(flagAllowedMsgKeys, []string{}, "Allowed msg keys")
+	cmd.Flags().String(flagAllowedRawMsgs, "", "Allowed raw msgs")


As above, this should be a StringSlice

alpe · 2022-11-08T21:03:24Z

x/wasm/client/cli/tx.go

+			}
+
+			var filter types.ContractAuthzFilterX
+			switch {


Filters are mutual exclusive within one grant. Better fail fast if more than one is set.

alpe · 2022-11-14T09:58:43Z

@Mergifyio rebase

mergify · 2022-11-14T09:59:23Z

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (7/37)
error: could not apply 9ae2fd3... Add contract authz proto
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 9ae2fd3... Add contract authz proto
CONFLICT (add/add): Merge conflict in x/wasm/types/authz.pb.go
Auto-merging x/wasm/types/authz.pb.go
CONFLICT (add/add): Merge conflict in proto/cosmwasm/wasm/v1/authz.proto
Auto-merging proto/cosmwasm/wasm/v1/authz.proto
Auto-merging docs/proto/proto-docs.md
CONFLICT (content): Merge conflict in docs/proto/proto-docs.md

err-code: FB22C

alpe

Good work! I added some nits and personal preferences. But nothing blocking!

alpe · 2022-11-14T10:01:49Z

x/wasm/client/cli/tx.go

+
+			var limit types.ContractAuthzLimitX
+			switch {
+			case maxFundsStr != "" && maxCalls != 0 && !noTokenTransfer:


alpe · 2022-11-14T10:04:46Z

x/wasm/client/cli/tx.go

+					return fmt.Errorf("max funds: %s", err)
+				}
+				limit = types.NewCombinedLimit(maxCalls, maxFunds...)
+			case maxFundsStr != "" && !noTokenTransfer:


nit: To be on the safe side, I would be very explicit: case maxFundsStr != "" && maxCalls == 0 && !noTokenTransfer:

alpe · 2022-11-14T10:05:46Z

x/wasm/client/cli/tx.go

+					return fmt.Errorf("max funds: %s", err)
+				}
+				limit = types.NewMaxFundsLimit(maxFunds...)
+			case maxCalls != 0 && noTokenTransfer:


to be on the safe side: maxCalls != 0 && noTokenTransfer && maxFundsStr == ""

alpe · 2022-11-14T10:08:43Z

x/wasm/client/cli/tx.go

+			case maxCalls != 0 && noTokenTransfer:
+				limit = types.NewMaxCallsLimit(maxCalls)
+			default:
+				limit = types.UndefinedLimit{}


you can fail fast here already

Suggested change

limit = types.UndefinedLimit{}

limit = errors.New("invalid limit setup")

alpe · 2022-11-14T10:12:20Z

x/wasm/client/cli/tx.go

+
+			var filter types.ContractAuthzFilterX
+			switch {
+			case allowAllMsgs && len(msgKeys) > 0 || allowAllMsgs && len(rawMsgs) > 0 || len(msgKeys) > 0 && len(rawMsgs) > 0:


👍 you cover all 3 cases here. I am a bit more used to read len(x) != 0 instead but all correct

alpe · 2022-11-14T10:12:47Z

x/wasm/client/cli/tx.go

+			var filter types.ContractAuthzFilterX
+			switch {
+			case allowAllMsgs && len(msgKeys) > 0 || allowAllMsgs && len(rawMsgs) > 0 || len(msgKeys) > 0 && len(rawMsgs) > 0:
+				return fmt.Errorf("cannot set more than one filter within one grant")


nit

Suggested change

return fmt.Errorf("cannot set more than one filter within one grant")

return errors.New("cannot set more than one filter within one grant")

alpe · 2022-11-14T10:14:02Z

x/wasm/client/cli/tx.go

+				}
+				filter = types.NewAcceptedMessagesFilter(msgs...)
+			default:
+				filter = &types.UndefinedFilter{}


nit: as above, just return an error here

alpe · 2022-11-14T10:17:22Z

x/wasm/client/cli/tx.go

+		Long: fmt.Sprintf(`Grant authorization to an address.
+Examples:
+$ %s tx grant <grantee_addr> execution <contract_addr> --allow-all-messages --maxCalls 1 --no-token-transfer --expiration 1667979596
+`, version.AppName),


Would it make sense to have some more concrete examples for the limits and filters? I think you can cover all in 3 examples

* Add contract authz proto * Implement contract autorization * Register contract authz * Add contract-authz tests * Consume gas for contract authz * Add contract authz cli * Update cli usage * Model spike * Add max funds limit * Redesign authz model * Start e2e test * Full e2e test * Add cli implementation for signle grant case * Restore file to avoid merge conflicts * Test filter and limits * Add allow-al-messages flag * Add cli implementation for signle grant case * Add allow-al-messages flag * Implement comments fixes * Test accept * Fix description * No linter warning * Fix flags and add example command * Fix lint error * Fix limits cli * Add cli implementation for signle grant case * Add allow-al-messages flag * Implement comments fixes * Fix flags and add example command * Fix lint error * Fix limits cli * Add cli implementation for signle grant case * Add allow-al-messages flag * Implement comments fixes * Fix flags and add example command * Fix lint error * Fix limits cli * Fix comments Co-authored-by: Giancarlos Salas <me@giansalex.dev> Co-authored-by: Alex Peters <alpe@users.noreply.github.com>

giansalex and others added 13 commits November 2, 2022 12:16

Add contract authz proto

9ae2fd3

Implement contract autorization

1358502

Register contract authz

8f30357

Add contract-authz tests

6dec6ad

Consume gas for contract authz

1cb6bd0

Add contract authz cli

fa235fd

Update cli usage

14cedff

Model spike

46aba00

Add max funds limit

443dbf4

Redesign authz model

8809e4f

Start e2e test

b1acbbd

Full e2e test

2d92e12

Add cli implementation for signle grant case

3d69c2e

alpe changed the base branch from main to 966_grants_redesign November 7, 2022 14:05

alpe reviewed Nov 8, 2022

View reviewed changes

pinosu and others added 10 commits November 8, 2022 11:01

Restore file to avoid merge conflicts

498c46f

Test filter and limits

3eb6e2d

Add allow-al-messages flag

f8b6403

Add cli implementation for signle grant case

bbe2086

Add allow-al-messages flag

28c1d94

Resolve rebase merge conflicts

84dd98c

Implement comments fixes

5c45d27

Test accept

40bdb6c

Fix description

5aed79c

No linter warning

cc8d9bd

alpe reviewed Nov 8, 2022

View reviewed changes

pinosu added 3 commits November 9, 2022 08:43

Fix flags and add example command

7a11526

Fix lint error

04d3b12

Fix limits cli

4724e4b

pinosu added 7 commits November 9, 2022 16:02

Add cli implementation for signle grant case

170b305

Add allow-al-messages flag

04987bc

Implement comments fixes

f8dd749

Fix flags and add example command

5b5ae89

Fix lint error

00de894

Fix limits cli

b5e6ee6

Rebase 966_grants_redesign

0797367

pinosu marked this pull request as ready for review November 11, 2022 09:06

Base automatically changed from 966_grants_redesign to main November 11, 2022 12:39

pinosu added 7 commits November 11, 2022 16:30

Add cli implementation for signle grant case

4ae3098

Add allow-al-messages flag

0e2f8eb

Implement comments fixes

a69f603

Fix flags and add example command

2faf404

Fix lint error

6732176

Fix limits cli

91cb542

Rebase to main branch

902ad19

alpe approved these changes Nov 14, 2022

View reviewed changes

Fix comments

0164d85

pinosu force-pushed the 966-grants_redesign_cli branch from 73c401b to 0164d85 Compare November 14, 2022 14:40

pinosu merged commit c0a482e into main Nov 14, 2022

pinosu deleted the 966-grants_redesign_cli branch November 14, 2022 15:06

This was referenced Nov 15, 2022

A better CLI support for authz #1093

Open

Authz module integration - more granularity for WasmExecuteMsg authorizations #803

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authz redesign: CLI implementation #1079

Authz redesign: CLI implementation #1079

pinosu commented Nov 7, 2022 •

edited

Loading

codecov bot commented Nov 7, 2022 •

edited

Loading

alpe left a comment

alpe Nov 8, 2022

alpe Nov 8, 2022

alpe Nov 8, 2022

alpe Nov 8, 2022

alpe Nov 8, 2022

alpe Nov 8, 2022

alpe left a comment

alpe Nov 8, 2022

alpe Nov 8, 2022

alpe Nov 8, 2022

alpe Nov 8, 2022

alpe commented Nov 14, 2022

mergify bot commented Nov 14, 2022

alpe left a comment

alpe Nov 14, 2022

alpe Nov 14, 2022

alpe Nov 14, 2022

alpe Nov 14, 2022

alpe Nov 14, 2022

alpe Nov 14, 2022

alpe Nov 14, 2022

alpe Nov 14, 2022

	limit = types.UndefinedLimit{}
	limit = errors.New("invalid limit setup")

	return fmt.Errorf("cannot set more than one filter within one grant")
	return errors.New("cannot set more than one filter within one grant")

Authz redesign: CLI implementation #1079

Authz redesign: CLI implementation #1079

Conversation

pinosu commented Nov 7, 2022 • edited Loading

codecov bot commented Nov 7, 2022 • edited Loading

Codecov Report

alpe left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpe left a comment

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

Choose a reason for hiding this comment

alpe commented Nov 14, 2022

mergify bot commented Nov 14, 2022

❌ Base branch update has failed

alpe left a comment

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

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

Choose a reason for hiding this comment

pinosu commented Nov 7, 2022 •

edited

Loading

codecov bot commented Nov 7, 2022 •

edited

Loading