fix!: remove time.now check from authz #10447
Conversation
* test: adding authz grant tests * fix TestCLITxGrantAuthorization/Invalid_expiration_time test case * comment out the test * reenable test
robert-zaremba
requested review from
aaronc,
ebuchman,
ValarDragon and
tac0turtle
| @@ -7,7 +7,7 @@ import ( | |||
| "github.com/cosmos/cosmos-sdk/x/authz/client/cli" | |||
| ) | |||
|
|
|||
| func ExecGrant(val *network.Validator, args []string) (testutil.BufferWriter, error) { | |||
| func CreateGrant(val *network.Validator, args []string) (testutil.BufferWriter, error) { | |||
There was a problem hiding this comment.
this function name (in tests) was misleading. It essentially creates a new grant and executes that message (grant creation). It doesn't execute on existing grant.
x/authz/authorization_grant.go
Outdated
| @@ -10,7 +10,10 @@ import ( | |||
| ) | |||
|
|
|||
| // NewGrant returns new Grant | |||
| func NewGrant(a Authorization, expiration time.Time) (Grant, error) { | |||
| func NewGrant(blockTime time.Time, a Authorization, expiration time.Time) (Grant, error) { | |||
There was a problem hiding this comment.
Why are we putting this in the constructor? Shouldn't we just do this within the MsgServer when creating a grant?
There was a problem hiding this comment.
We are validating the grant, here authorization + expiration relation. The motivation to have it in NewGrant is to do it once and make sure it's always validated.
Moving it outside of the NewGrant would make sense if it would be a private method... but then we will need another function because other packages are using NewGrant.
So. what do you think ?
There was a problem hiding this comment.
On the other hand NewGrant is used only in one place (not counting tests)
There was a problem hiding this comment.
I would rather just do it the correct way and valid block time when we're actually in a block... but not going to block either
There was a problem hiding this comment.
OK, I reworked the validation... was thinking about creating Grant.Validate(now) method, but it also wasn't perfect.
|
@robert-zaremba can we get this merged? |
|
ping |
robert-zaremba
removed
the
A:automerge
| suite.Require().Equal(expected[i].opMsgName, operationMsg.Name, "operation Msg name should be the same") | ||
| suite.Require().Equal(expected[i].weight, w.Weight(), "test: %d, weight should be the same", i) | ||
| suite.Require().Equal(expected[i].opMsgRoute, operationMsg.Route, "test: %d, route should be the same", i) | ||
| suite.Require().Equal(expected[i].opMsgName, operationMsg.Name, "test: %d, operation Msg name should be the same", i) |
There was a problem hiding this comment.
we were not checking the results, all tests were failing , and wrong route was used.... let's solve it in other issue
tac0turtle
removed
the
A:automerge
tac0turtle
force-pushed
the
robert/backport-0.44.2-master
branch
from
f65bd0b to
940e165
Compare
tac0turtle
added
the
A:automerge
|
seems sims are still failing. This is a blocker for 0.46 releases. |
tac0turtle
changed the title
This reverts commit 077154a.
Description
Backport v0.44.2 fix
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
!in the type prefix if API or client breaking change