External feegrants #1338

KyleMoser · 2023-11-20T21:45:11Z

Allows feegrants where relayer does not have the Granter private key/mnemonic
This means the grantees are authorized externally (someone else owns the Granter key)

jtieri

i'm not intimately familiar with feegrant as a feature so i could be missing some context that would lead me to miss the validity of some implementation detail but I took a couple passes and everything looks mostly good besides for a few comments i had.

perhaps @boojamya, @vimystic or @agouin would care to take a pass as well?

jtieri · 2023-12-07T01:42:32Z

relayer/chains/cosmos/bech32_hack.go

+}
+
+// SetSDKContext sets the SDK config to the given bech32 prefixes
+func SetSDKConfigContext(prefix string) func() {


if this helper method is not used elsewhere we could change it from exported to unexported.

jtieri · 2023-12-07T17:33:18Z

cmd/feegrant.go

 					feegrantErr = prov.ConfigureWithGrantees(grantees, granterKey)
+				} else {
+					fmt.Println("!! ConfigureWithExternalGranter !!")


this looks like it may be debug output?

jtieri · 2023-12-07T17:36:29Z

relayer/chains/cosmos/feegrant.go

+	var granterAddr string
+	var granterAcc types.AccAddress
+	var err error


not necessary but this could just be

var ( granterAddr string granterAcc types.AccAddress err error )

jtieri · 2023-12-07T17:41:56Z

relayer/chains/cosmos/feegrant.go

 			fmt.Printf("Grant will be created on chain for granter %s and grantee %s\n", granterAddr, granteeAddr)
 			grantMsg, err := cc.getMsgGrantBasicAllowance(granterAcc, granteeAcc)
 			if err != nil {
 				return nil, err
 			}
 			msgs = append(msgs, grantMsg)
+		} else if !hasGrant {
+			fmt.Printf("WARN: External granter %s has no on-chain feegrant for grantee %s\n", granterAddr, granteeAddr)


It seems like we are going for level based logging here with the WARN, we should have access to the ChainProvider's zap.Logger here so you could use that to have a structured log at warn level.

We have a doc describing our logging conventions here. This could be used for all the logs being printed to stdout so that the logs are consistent looking in an operators terminal

KyleMoser · 2023-12-11T20:08:55Z

i'm not intimately familiar with feegrant as a feature so i could be missing some context that would lead me to miss the validity of some implementation detail but I took a couple passes and everything looks mostly good besides for a few comments i had.

perhaps @boojamya, @vimystic or @agouin would care to take a pass as well?

Thanks for the feedback. Yeah, this is a minor update so everything should be the same functionality wise. But it's a very error-prone feature so I appreciate the review. I will fix the issues you suggested; did not mean to leave the Printfs in there, that was just debugging.

ertemann · 2023-12-29T13:58:48Z

@KyleMoser did you happen to have time to finalise this PR? i dont have the skills really to finish this myself but we would love this feature to be implemented :)

dylanschultzie · 2024-01-16T18:36:04Z

I can confirm this works.

relayer address: https://finder.kujira.network/kaiyo-1/address/kujira18xrruhq5r246mwk0yj9elnn3mte8xa9u3ae4pk

tx: https://finder.kujira.network/kaiyo-1/tx/52B8E62E0D1546DC79209DB85EAA233B95A2594FEFDE49594059BFC6DE55CFCE

feegrant config:

feegrants:
                granter: kujira1vkje22mayn72r0a7kna6agv0sqm6k94ry9k6dd
                external_granter: true
                grantees:
                    - default

json:

    {
      "type": "tx",
      "attributes": [
        {
          "key": "fee",
          "value": "3717ukuji",
          "index": true
        },
        {
          "key": "fee_payer",
          "value": "kujira1vkje22mayn72r0a7kna6agv0sqm6k94ry9k6dd",
          "index": true
        }
      ]
    },

jtieri

Mostly just had a bunch of nit picks around logging and our style guide.

One question I do have though is if the race condition that is being detected in the test case is something within our control to fix?

jtieri · 2024-01-24T18:19:21Z

cmd/feegrant.go

 			}

 			if delete {
-				fmt.Printf("Deleting %s feegrant configuration\n", chain)
+				a.log.Info("deleting feegrant configuration", zap.String("chain", chain))


nit: logs should begin with an uppercase letter https://github.com/cosmos/relayer/blob/main/docs/logging.md#usage

jtieri · 2024-01-24T18:24:27Z

cmd/feegrant.go

@@ -190,7 +205,7 @@ func feegrantBasicGrantsCmd(a *appState) *cobra.Command {

 			granterAcc, err := prov.AccountFromKeyOrAddress(keyNameOrAddress)
 			if err != nil {
-				fmt.Printf("Error retrieving account from key '%s'\n", keyNameOrAddress)
+				a.log.Error("unknown account", zap.String("key or address", keyNameOrAddress))


nit: logs should begin with an uppercase letter.

Also, should we log the returned error here as well?

jtieri · 2024-01-24T18:26:10Z

relayer/chains/cosmos/feegrant.go

 			if isValidGrant(feegrantAllowance.(*feegrant.BasicAllowance)) {
 				validGrants = append(validGrants, grant)
 			}
 		default:
-			fmt.Printf("Ignoring grant type %s for granter %s and grantee %s\n", grant.Allowance.TypeUrl, grant.Granter, grant.Grantee)
+			cc.log.Debug("ignoring grant",


nit: logs should begin with an uppercase letter

jtieri · 2024-01-24T18:26:22Z

relayer/chains/cosmos/feegrant.go

@@ -97,7 +99,10 @@ func (cc *CosmosProvider) GetGranteeValidBasicGrants(granteeKey string) ([]*feeg
 					validGrants = append(validGrants, grant)
 				}
 			default:
-				fmt.Printf("Ignoring grant type %s for granter %s and grantee %s\n", grant.Allowance.TypeUrl, grant.Granter, grant.Grantee)
+				cc.log.Debug("ignoring grant",


nit: logs should begin with an uppercase letter

jtieri · 2024-01-24T18:29:01Z

relayer/chains/cosmos/feegrant.go

 			grantMsg, err := cc.getMsgGrantBasicAllowance(granterAcc, granteeAcc)
 			if err != nil {
 				return nil, err
 			}
 			msgs = append(msgs, grantMsg)
+		} else if !hasGrant {
+			cc.log.Warn("Missing feegrant", zap.String("external granter", granterAddr), zap.String("grantee", granteeAddr))


nit: log field names should be snake_case https://github.com/cosmos/relayer/blob/main/docs/logging.md#usage

jtieri · 2024-01-24T18:29:11Z

relayer/chains/cosmos/feegrant.go

 				return nil, err
 			} else if txResp != nil && txResp.TxResponse != nil && txResp.TxResponse.Code != 0 {
-				fmt.Printf("Submitting grants for granter %s failed. Code: %d, TX hash: %s\n", granterKey, txResp.TxResponse.Code, txResp.TxResponse.TxHash)
+				cc.log.Warn("Feegrant TX failed", zap.String("TX hash", txResp.TxResponse.TxHash), zap.Uint32("code", txResp.TxResponse.Code))


nit: log field names should be snake_case https://github.com/cosmos/relayer/blob/main/docs/logging.md#usage

jtieri · 2024-01-24T18:29:21Z

relayer/chains/cosmos/feegrant.go

 				return nil, fmt.Errorf("could not configure feegrant for granter %s", granterKey)
 			}

-			fmt.Printf("TX succeeded, %d new grants configured, %d grants already in place. TX hash: %s\n", grantsNeeded, numGrantees-grantsNeeded, txResp.TxResponse.TxHash)
+			cc.log.Info("Feegrant succeeded", zap.Int("new grants", grantsNeeded), zap.Int("existing grants", numGrantees-grantsNeeded), zap.String("TX hash", txResp.TxResponse.TxHash))


nit: log field names should be snake_case https://github.com/cosmos/relayer/blob/main/docs/logging.md#usage

jtieri · 2024-01-24T18:29:44Z

relayer/chains/cosmos/feegrant.go

 	if granterKey == "" {
 		granterKey = cc.PCfg.Key
 	}
 	granterAddr, err := cc.GetKeyAddressForKey(granterKey)
 	if err != nil {
-		fmt.Printf("ChainClient FeeGranter misconfiguration: %s", err.Error())
+		cc.log.Error("Unknown granter", zap.String("key name", granterKey))


nit: log field names should be snake_case https://github.com/cosmos/relayer/blob/main/docs/logging.md#usage

jtieri · 2024-01-24T18:29:55Z

relayer/chains/cosmos/feegrant.go

 	if granterKey == "" {
 		granterKey = cc.PCfg.Key
 	}

 	granterAddr, err := cc.GetKeyAddressForKey(granterKey)
 	if err != nil {
-		fmt.Printf("ChainClient FeeGranter misconfiguration: %s", err.Error())
+		cc.log.Error("Unknown granter", zap.String("key name", granterKey))


nit: log field names should be snake_case https://github.com/cosmos/relayer/blob/main/docs/logging.md#usage

jtieri · 2024-01-24T18:29:59Z

relayer/chains/cosmos/feegrant.go

 		return err
 	}

 	for _, grantee := range cc.PCfg.FeeGrants.ManagedGrantees {
 		granteeAddr, err := cc.GetKeyAddressForKey(grantee)

 		if err != nil {
-			fmt.Printf("Misconfiguration for grantee %s. Error: %s\n", grantee, err.Error())
+			cc.log.Error("Unknown grantee", zap.String("key name", grantee))


nit: log field names should be snake_case https://github.com/cosmos/relayer/blob/main/docs/logging.md#usage

KyleMoser · 2024-01-24T18:49:25Z

Mostly just had a bunch of nit picks around logging and our style guide.

One question I do have though is if the race condition that is being detected in the test case is something within our control to fix?

Thanks, I will fix those logs tonight. No, the race condition cannot be fixed without patching the SDK. Fortunately, the race condition only affects tests, it will not impact rly.

jtieri · 2024-01-24T18:57:17Z

Mostly just had a bunch of nit picks around logging and our style guide.
One question I do have though is if the race condition that is being detected in the test case is something within our control to fix?

Thanks, I will fix those logs tonight. No, the race condition cannot be fixed without patching the SDK. Fortunately, the race condition only affects tests, it will not impact rly.

That's what i thought you had mentioned before but wasn't 100% sure, figured I would ask!

Since the remaining comments I had were all regarding logs I'll just approve this so you aren't blocked by me later when you make your changes.

jtieri

🚢 🚢 🚢

Kyle added 2 commits November 20, 2023 16:42

Allow feegrants where granter is external (relayer does not have key)

7ea0c47

Merge branch 'main' of ssh://github.com/cosmos/relayer

453d51d

jtieri reviewed Dec 7, 2023

View reviewed changes

linting/logging

91883e4

Kyle added 2 commits January 22, 2024 13:29

Added feegrant command gas flag

89584b4

merged with upstream

9b7af82

KyleMoser marked this pull request as ready for review January 22, 2024 19:11

KyleMoser requested review from jackzampolin, boojamya and mark-rushakoff as code owners January 22, 2024 19:11

Added doc for external feegrant config

122efcb

jtieri reviewed Jan 24, 2024

View reviewed changes

jtieri approved these changes Jan 24, 2024

View reviewed changes

Kyle added 5 commits January 24, 2024 23:32

Fix logging to conform to style guide

f525e88

Merge upstream

77bcf26

Lock on read bech32 address map

46dbb93

Lock on read bech32 address map

d17f1c6

Lock on read bech32 address map

8d9de86

agouin merged commit 7cb083c into cosmos:main Jan 29, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External feegrants #1338

External feegrants #1338

KyleMoser commented Nov 20, 2023

jtieri left a comment

jtieri Dec 7, 2023

jtieri Dec 7, 2023

jtieri Dec 7, 2023

jtieri Dec 7, 2023

KyleMoser commented Dec 11, 2023

ertemann commented Dec 29, 2023

dylanschultzie commented Jan 16, 2024 •

edited

Loading

jtieri left a comment

jtieri Jan 24, 2024

jtieri Jan 24, 2024

jtieri Jan 24, 2024

jtieri Jan 24, 2024

jtieri Jan 24, 2024

jtieri Jan 24, 2024

jtieri Jan 24, 2024

jtieri Jan 24, 2024

jtieri Jan 24, 2024

jtieri Jan 24, 2024

KyleMoser commented Jan 24, 2024

jtieri commented Jan 24, 2024

jtieri left a comment

External feegrants #1338

External feegrants #1338

Conversation

KyleMoser commented Nov 20, 2023

jtieri 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

KyleMoser commented Dec 11, 2023

ertemann commented Dec 29, 2023

dylanschultzie commented Jan 16, 2024 • edited Loading

jtieri 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyleMoser commented Jan 24, 2024

jtieri commented Jan 24, 2024

jtieri left a comment

Choose a reason for hiding this comment

dylanschultzie commented Jan 16, 2024 •

edited

Loading