feat!: make grant expiration optional #11060

robert-zaremba · 2022-01-28T19:54:48Z

Description

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

…tion

codecov · 2022-03-17T11:04:02Z

Codecov Report

Merging #11060 (0bce508) into master (20735e7) will increase coverage by 0.03%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master   #11060      +/-   ##
==========================================
+ Coverage   65.86%   65.89%   +0.03%     
==========================================
  Files         675      675              
  Lines       69773    69789      +16     
==========================================
+ Hits        45955    45988      +33     
+ Misses      21118    21100      -18     
- Partials     2700     2701       +1

Impacted Files	Coverage Δ
x/authz/keeper/keys.go	`95.08% <ø> (ø)`
x/authz/simulation/operations.go	`66.97% <57.89%> (+1.53%)`	⬆️
x/authz/keeper/keeper.go	`69.75% <65.51%> (-0.44%)`	⬇️
types/coin.go	`93.47% <100.00%> (+0.06%)`	⬆️
types/query/filtered_pagination.go	`81.57% <100.00%> (+0.24%)`	⬆️
x/authz/authorization_grant.go	`65.71% <100.00%> (-1.86%)`	⬇️
x/authz/migrations/v046/store.go	`70.45% <100.00%> (+1.40%)`	⬆️
x/authz/msgs.go	`62.09% <100.00%> (ø)`
x/authz/simulation/genesis.go	`94.44% <100.00%> (+0.69%)`	⬆️
crypto/keys/internal/ecdsa/privkey.go	`82.45% <0.00%> (-1.76%)`	⬇️
... and 5 more

robert-zaremba · 2022-03-18T00:08:42Z

x/authz/client/cli/tx.go

@@ -173,10 +173,22 @@ Examples:
 	cmd.Flags().String(FlagSpendLimit, "", "SpendLimit for Send Authorization, an array of Coins allowed spend")
 	cmd.Flags().StringSlice(FlagAllowedValidators, []string{}, "Allowed validators addresses separated by ,")
 	cmd.Flags().StringSlice(FlagDenyValidators, []string{}, "Deny validators addresses separated by ,")
-	cmd.Flags().Int64(FlagExpiration, time.Now().AddDate(1, 0, 0).Unix(), "The Unix timestamp. Default is one year.")
+	cmd.Flags().Int64(FlagExpiration, 0, "Expire time as Unix timestamp. Set zero (0) for no expiry. Default is 0.")


I've changed the default here to zero = no expiration time.
Maybe we would like to explicitly require the value? In that case:

0 would be not allowed

-1 = no expire date

I think 0 == no expiration time makes the most sense.

Alternatively we can do:

0 == no expiry

default is -1

all negative values are not allowed

Alternatively we can do: 0 == no expiry; default is -1

If we want to make the CLI param required, the I prefer -1 as no expire, this is because 0 would mean "default", and we don't want default (from the user perspective).

all negative values are not allowed

Now, all values < blocktime().unix() are not allowed , except zero.

i was thinking:

Suggested change

cmd.Flags().Int64(FlagExpiration, 0, "Expire time as Unix timestamp. Set zero (0) for no expiry. Default is 0.")

cmd.Flags().Int64(FlagExpiration, -1, "Expire time as Unix timestamp. Set zero (0) for no expiry.")

But tbh -1 = no expire date sounds good to me too, if there's a help message.

robert-zaremba · 2022-03-18T12:12:42Z

x/authz/simulation/operations.go

-			return simtypes.NoOpMsg(authz.ModuleName, TypeMsgGrant, "past time"), nil, nil
+		var expiration *time.Time
+		t1 := simtypes.RandTimestamp(r)
+		if !t1.Before(ctx.BlockTime()) {


bummer! calling a rand here will cause sim tests to fail :( This is because our simulation tests don't simulate much - they are almost 'pre defined': we have one grant message, one exec and one revoke. it's basically one premade scenario with lot of boilerplate.

…tion

blushi

Implementation looks good to me, pending simulations

x/authz/keeper/genesis_test.go

x/authz/simulation/operations.go

amaury1093

Approving pending sims test fix, I suppose related to #11060 (comment)

atheeshp

lgtm, except sims & few nits.

proto/cosmos/authz/v1beta1/authz.proto

x/authz/keeper/grpc_query_test.go

…tion

robert-zaremba · 2022-03-24T10:34:26Z

Wow, there is a weird convolution, no idea why this test is failing:

    validator_test.go:476: 
                Error Trace:    validator_test.go:476
                Error:          Not equal: 
                                expected: 2
                                actual  : 3
                Test:           TestGetValidatorSortingMixed

robert-zaremba · 2022-03-24T10:36:12Z

It is also failing in master

amaury1093 · 2022-03-24T11:49:56Z

@robert-zaremba It's fixed on master now

robert-zaremba added the C:x/authz label Jan 28, 2022

robert-zaremba added this to the v0.46 milestone Jan 28, 2022

aleem1314 mentioned this pull request Feb 7, 2022

Update authz expire to make it optional #11095

Closed

4 tasks

robert-zaremba force-pushed the robert/authz-expiration branch from 932bb6d to ecc83c1 Compare February 24, 2022 11:34

github-actions bot removed the C:x/authz label Feb 24, 2022

robert-zaremba added 2 commits February 28, 2022 16:57

feat!: make grant expiration optional

90570f3

proto regeneration

5201088

robert-zaremba force-pushed the robert/authz-expiration branch from ecc83c1 to 5201088 Compare February 28, 2022 15:57

github-actions bot added the C:x/authz label Feb 28, 2022

robert-zaremba added 3 commits March 2, 2022 15:53

update grant msg

5ede9ca

update authz.GrantAuthorization

df83f4b

update simulations

37e8c7b

github-actions bot added C:CLI C:Simulations labels Mar 2, 2022

robert-zaremba added 9 commits March 2, 2022 22:41

fix SaveGrant

f8602da

fixing tests

0689d24

fixing tests2

4e8e5a4

update tests

a6a52b5

Merge remote-tracking branch 'origin/master' into robert/authz-expira…

458aba1

…tion

fix keeper_test

580acb3

fix keeper tests

7e6ebeb

fix nil pointer

9f5e473

fix sim test compilation

b4c12c5

update authz grant CLI

ed46c73

robert-zaremba commented Mar 18, 2022

View reviewed changes

robert-zaremba added 3 commits March 18, 2022 01:53

add null expire time to authz simulations

d25a473

update sime test

5224e6d

changelog

a309dfc

robert-zaremba marked this pull request as ready for review March 18, 2022 01:08

add comment

2c4025b

robert-zaremba force-pushed the robert/authz-expiration branch from 79649a3 to 2c4025b Compare March 18, 2022 11:23

robert-zaremba added 2 commits March 18, 2022 12:53

fixing sim tests

cc6cb8f

cleanup sim tests

7375a06

robert-zaremba requested a review from amaury1093 March 18, 2022 12:06

robert-zaremba commented Mar 18, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into robert/authz-expira…

cf7ab0d

…tion

robert-zaremba force-pushed the robert/authz-expiration branch from f8284d0 to cf7ab0d Compare March 18, 2022 12:43

blushi reviewed Mar 18, 2022

View reviewed changes

x/authz/keeper/genesis_test.go Show resolved Hide resolved

x/authz/simulation/operations.go Outdated Show resolved Hide resolved

removed debug logs

9317533

amaury1093 approved these changes Mar 18, 2022

View reviewed changes

atheeshp approved these changes Mar 21, 2022

View reviewed changes

proto/cosmos/authz/v1beta1/authz.proto Show resolved Hide resolved

x/authz/keeper/grpc_query_test.go Outdated Show resolved Hide resolved

likhita-809 approved these changes Mar 21, 2022

View reviewed changes

robert-zaremba added 5 commits March 22, 2022 17:45

Merge remote-tracking branch 'origin/master' into robert/authz-expira…

84e95ff

…tion

fix init genesis

bdc40b2

fix dispatch

cb2e328

adding comment

0c1b02e

Merge remote-tracking branch 'origin/master' into robert/authz-expira…

7d3f8c4

…tion

Merge branch 'master' into robert/authz-expiration

dafb921

Merge branch 'master' into robert/authz-expiration

5eb446e

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 24, 2022

rename createCoinAuthorization

0bce508

mergify bot merged commit c1b6ace into master Mar 24, 2022

mergify bot deleted the robert/authz-expiration branch March 24, 2022 18:27

amaury1093 mentioned this pull request May 20, 2022

Manual Testing for v0.46 #11880

Closed

72 tasks

LampardNguyen234 mentioned this pull request Aug 15, 2023

deps: bump cosmos-sdk v0.46 and evmos v12 AstraProtocol/astra#209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: make grant expiration optional #11060

feat!: make grant expiration optional #11060

robert-zaremba commented Jan 28, 2022 •

edited by amaury1093

Loading

codecov bot commented Mar 17, 2022 •

edited

Loading

robert-zaremba Mar 18, 2022

amaury1093 Mar 18, 2022

robert-zaremba Mar 18, 2022

amaury1093 Mar 18, 2022

robert-zaremba Mar 18, 2022

blushi left a comment

amaury1093 left a comment

atheeshp left a comment

robert-zaremba commented Mar 24, 2022

robert-zaremba commented Mar 24, 2022

amaury1093 commented Mar 24, 2022

	cmd.Flags().Int64(FlagExpiration, 0, "Expire time as Unix timestamp. Set zero (0) for no expiry. Default is 0.")
	cmd.Flags().Int64(FlagExpiration, -1, "Expire time as Unix timestamp. Set zero (0) for no expiry.")

feat!: make grant expiration optional #11060

feat!: make grant expiration optional #11060

Conversation

robert-zaremba commented Jan 28, 2022 • edited by amaury1093 Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Mar 17, 2022 • edited Loading

Codecov Report

robert-zaremba Mar 18, 2022

Choose a reason for hiding this comment

amaury1093 Mar 18, 2022

Choose a reason for hiding this comment

robert-zaremba Mar 18, 2022

Choose a reason for hiding this comment

amaury1093 Mar 18, 2022

Choose a reason for hiding this comment

robert-zaremba Mar 18, 2022

Choose a reason for hiding this comment

blushi left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

atheeshp left a comment

Choose a reason for hiding this comment

robert-zaremba commented Mar 24, 2022

robert-zaremba commented Mar 24, 2022

amaury1093 commented Mar 24, 2022

robert-zaremba commented Jan 28, 2022 •

edited by amaury1093

Loading

codecov bot commented Mar 17, 2022 •

edited

Loading