Axelar Cork #209

mvid · 2023-04-04T23:49:07Z

Closes: #XXX

Description

cbrit · 2023-04-05T17:36:04Z

proto/axelar-cork/v1/cork.proto

+
+option go_package = "github.com/peggyjv/sommelier/x/axelarcork/types";
+
+// todo: do we need Axelar in the name or should we depend on the namespace?


I'm a fan of the specificity

axelarcorktypes.AxelarCork.AxelarTargetContractAddress

Ashes to ashes, java to java

proto/axelar-cork/v1/proposal.proto

cbrit

Incomplete pass

cbrit · 2023-05-03T18:55:49Z

proto/buf.yaml

+name: buf.build/cosmwasm/xiond
+deps:
+  - buf.build/cosmos/cosmos-proto
+  - buf.build/cosmos/cosmos-sdk:v0.47.0


Is using the 0.47.0 buf image while we're using 0.45 going to cause issues?

Not sure. It seems to be fine so far? I don't know of any breaking changes in proto definitions

Bumping this so we follow up on it.

This is a new file, that the current protoc-gen.sh does not use. It was part of my attempt to update proto generation to use the new docker method, which seems like it won't be pursued. The correct option is likely to delete all of these proto/buf* files. Should I do that?

cbrit · 2023-05-03T18:56:25Z

proto/buf.md

@@ -0,0 +1,3 @@
+# Protobufs
+
+This is the public protocol buffers API for [Wasmd](https://github.com/CosmWasm/wasmd).


do we need this file?

Don't know, but I changed it to Sommelier

cbrit · 2023-05-03T19:27:25Z

proto/axelar-cork/v1/cork.proto

+message ChainConfiguration {
+  string name           = 1;
+  uint64 id             = 2;
+  string vote_threshold = 3 [


what's the rationale behind chain-specific thresholds?

Unclear. It's a carryover from Axelar v1, where we had a vote threshold for Ethereum. It could be moved to a Param, but @EricBolten and I couldn't really think of a reason to go in either direction, so left it here. More configuration seems better?

Per our conversations around fixes in the original cork module, we may want to drop this as a parameter and stick with a hardcoded vote threshold value of consensus.

proto/axelar-cork/v1/event.proto

proto/axelar-cork/v1/query.proto

x/axelarcork/keeper/proposal_handler.go

proto/axelar-cork/v1/cork.proto

x/axelarcork/handler.go

EricBolten

One more pass of questions and comments.

EricBolten · 2023-07-31T14:03:58Z

x/axelarcork/keeper/genesis.go


-			k.SetScheduledAxelarCork(ctx, config.Id, scheduledCork.BlockHeight, valAddr, *scheduledCork.Cork)
+	for _, scheduledCork := range gs.ScheduledCorks.ScheduledCorks {
+		valAddr, err := sdk.ValAddressFromHex(scheduledCork.Validator)


I would think the validator address in a scheduled cork would be bech32 and not hex, no?

EricBolten · 2023-07-31T14:06:38Z

x/axelarcork/keeper/keeper.go

@@ -150,7 +151,7 @@ func (k Keeper) GetScheduledAxelarCorks(ctx sdk.Context, chainID uint64) []*type
 			Validator:   val.String(),
 			Cork:        &cork,
 			BlockHeight: blockHeight,
-			Id:          id,
+			Id:          hex.EncodeToString(id),


Looking at this commit, we've updated the output encoding, but are there any queries that rely on the ID as input which were expecting bytes before and now need to convert from a hex string into bytes?

EricBolten · 2023-07-31T14:11:02Z

x/axelarcork/client/cli/query.go


 	return cmd
 }

 func queryScheduledBlockHeights() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:     "scheduled-block-heights",
+		Use:     "scheduled-block-heights [chain-id]",


Looking at some of these commands (beyond just this one) and wondering if they wouldn't be better with chain-id as an optional argument and if it's missing, returning a list with all chain IDs included. I guess the only thing that could make that kind of ugly is if the return values don't have embedded chain IDs, making the response object unclear.

EricBolten · 2023-07-31T14:11:52Z

x/axelarcork/client/cli/query.go


 	return cmd
 }

 func queryScheduledCorksByID() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:     "scheduled-corks-by-id",
+		Use:     "scheduled-corks-by-id [chain-id]",


Chain ID is part of the cork ID hash now. They should be unique regardless of the chain ID, so for queries using cork IDs we shouldn't need the chain ID.

EricBolten · 2023-07-31T14:12:04Z

x/axelarcork/client/cli/query.go


 	return cmd
 }

 func queryCorkResult() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:     "cork-result",
+		Use:     "cork-result [chain-id] [cork-id]",


Here as well re: cork ID.

EricBolten · 2023-07-31T14:19:06Z

x/axelarcork/keeper/abci.go

+			"height", fmt.Sprintf("%d", ctx.BlockHeight()),
+			"chain id", config.Id)
+
+		timeoutHeight := k.GetParamSet(ctx).CorkTimeoutBlocks + uint64(ctx.BlockHeight())


Shouldn't this be current block height subtracting the timeout param? Adding it to the current block height will always produce one in the future.

My bad, fixed

EricBolten · 2023-07-31T14:25:07Z

x/axelarcork/keeper/keeper.go

+func (k Keeper) DeleteWinningAxelarCork(ctx sdk.Context, chainID uint64, c types.AxelarCork) {
+
+	k.IterateWinningAxelarCorks(ctx, chainID, func(contract common.Address, blockHeight uint64, cork types.AxelarCork) (stop bool) {
+		if c.Equals(cork) {


Should we be concerned with also checking the contract address here?

EricBolten · 2023-07-31T14:26:21Z

x/axelarcork/keeper/keeper.go

+		keyPair := bytes.NewBuffer(iter.Key())
+		keyPair.Next(1) // trim prefix byte
+		keyPair.Next(8) // trim chain ID
+		blockHeight := binary.BigEndian.Uint64(keyPair.Next(8))


I understand this design was to not change the storage type, but given that we've landed on one enqueued call per contract address, is there a reason we couldn't switch this to an AxelarScheduledCork type and just have the block height stored with it, and do the lookup based on the chain ID and contract address?

EricBolten · 2023-07-31T14:28:10Z

x/axelarcork/types/params.go

+		GmpAccount:        "",
+		ExecutorAccount:   "",
+		TimeoutDuration:   0,
+		CorkTimeoutBlocks: 100,


We probably want to set this to a larger value, but I can check in with Seven Seas to get their opinion.

I agree, switching it to 10000

EricBolten · 2023-07-31T14:30:06Z

.golangci.yml

@@ -8,7 +8,7 @@ linters:
  enable:
    - bodyclose
    - deadcode
-    - depguard
+    # - depguard


Any reason we wanted this commented out of the linter?

It makes it easier for me to lint locally. For some reason, this line isn't enforced on CI, and I am not sure why we enforce it in the local lint config

github-actions · 2023-09-15T00:17:48Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions · 2023-10-31T00:18:35Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

clean build

54f4e05

mvid requested review from zmanian, EricBolten and cbrit April 4, 2023 23:49

mvid changed the base branch from main to bolten/v6-rc1 April 4, 2023 23:50

bring over axelar module

e726d40

mvid temporarily deployed to CI April 4, 2023 23:56 — with GitHub Actions Inactive

cbrit reviewed Apr 5, 2023

View reviewed changes

proto/axelar-cork/v1/proposal.proto Outdated Show resolved Hide resolved

mvid added 10 commits April 11, 2023 20:03

redo axelar corks with cork v2

d190bfb

rpc endpoints for hitting axelar

e93514c

cleanup for CLI

bd81a11

clean up CLI

83cde22

proposal trasnfered from gravity

ec2739e

typo

927c250

update for heighliner

a885feb

genesis updates

f2877b3

governance

465c4c2

rename msg field

ee5fe9d

cbrit reviewed May 5, 2023

View reviewed changes

mvid added 8 commits May 5, 2023 13:25

some review updates

a7bacc3

more review updates

be1bffb

even more review updates!

530fe58

proposal updates

9e18aad

chain configs query

4cc1105

comment updates

29fe77e

fix issue with ID in key parsing

0eeb0b2

add staking mocks

b438612

mvid added 4 commits July 26, 2023 19:07

more changes

46ec04f

cleanup local script changes for protos

3dc9011

encode id to string on result retrieval

5a7b6d8

revert cellarfee protos

40bb3c0

mvid temporarily deployed to CI July 26, 2023 17:38 — with GitHub Actions Inactive

test update for hex

30d1901

mvid temporarily deployed to CI July 26, 2023 17:43 — with GitHub Actions Inactive

cork timeouts

5fb45e9

mvid had a problem deploying to CI July 28, 2023 18:31 — with GitHub Actions Failure

updates for unused cancel msg

c19a92c

mvid temporarily deployed to CI July 28, 2023 18:41 — with GitHub Actions Inactive

EricBolten reviewed Jul 31, 2023

View reviewed changes

several review comments

087ca62

mvid temporarily deployed to CI July 31, 2023 18:26 — with GitHub Actions Inactive

github-actions bot added the Stale label Sep 15, 2023

EricBolten removed the Stale label Sep 15, 2023

github-actions bot added the Stale label Oct 31, 2023

github-actions bot closed this Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Axelar Cork #209

Axelar Cork #209

mvid commented Apr 4, 2023

cbrit Apr 5, 2023

mvid May 3, 2023

cbrit left a comment

cbrit May 3, 2023

mvid May 5, 2023

EricBolten Jul 13, 2023

mvid Jul 20, 2023

cbrit May 3, 2023

mvid May 5, 2023

cbrit May 3, 2023

mvid May 5, 2023

EricBolten Jun 15, 2023

EricBolten left a comment

EricBolten Jul 31, 2023

mvid Jul 31, 2023

EricBolten Jul 31, 2023

EricBolten Jul 31, 2023

EricBolten Jul 31, 2023

EricBolten Jul 31, 2023

EricBolten Jul 31, 2023

mvid Jul 31, 2023

EricBolten Jul 31, 2023

EricBolten Jul 31, 2023

EricBolten Jul 31, 2023

mvid Jul 31, 2023

EricBolten Jul 31, 2023

mvid Jul 31, 2023

github-actions bot commented Sep 15, 2023

github-actions bot commented Oct 31, 2023


		option go_package = "github.com/peggyjv/sommelier/x/axelarcork/types";

		// todo: do we need Axelar in the name or should we depend on the namespace?

		@@ -0,0 +1,3 @@
		# Protobufs

		This is the public protocol buffers API for [Wasmd](https://github.com/CosmWasm/wasmd).

Axelar Cork #209

Axelar Cork #209

Conversation

mvid commented Apr 4, 2023

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbrit 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

EricBolten 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

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

github-actions bot commented Sep 15, 2023

github-actions bot commented Oct 31, 2023