feat(go/util/jwt): implement signing jwt using ADR36 spec #236
Conversation
WalkthroughAdds an off-chain signing proto/message (MsgSignData) and its Cosmos SDK msg, introduces a new JWT signing method ES256KADR36 that uses ADR-36 style sign bytes, adds signer/verifier abstractions, refactors ES256K path to use them, updates JWT tests/fixtures, and extends provider docs with workloads entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant JWT as ES256KADR36
participant Signer as SignerI (keyring)
participant Offchain as StdSignBytes(MsgSignData)
App->>JWT: Sign(signingString, signer)
note right of JWT #DDEBF7: Wrap signingString in MsgSignData (signer,data)
JWT->>Offchain: Build StdSignBytes(chainID,..., msgs=[MsgSignData])
Offchain-->>JWT: signBytes
JWT->>Signer: SignByAddress(addr, signBytes)
Signer-->>JWT: signature
JWT-->>App: signature segment
sequenceDiagram
autonumber
participant App
participant Parser as jwt.Parse(WithValidMethods)
participant Method as ES256K / ES256KADR36
participant Ver as VerifyI
App->>Parser: Parse(token, keyfunc)
Parser->>Method: Verify(signingString, signature, key)
alt ES256KADR36
note right of Method #F7F3DE: Recreate MsgSignData -> StdSignBytes
else ES256K
note right of Method #F7F3DE: Verify raw signingString
end
Method->>Ver: Pubkey().VerifySignature(message, signature)
Method-->>Parser: ok / jwt.ErrTokenSignatureInvalid
Parser-->>App: claims or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (6)go/util/jwt/signer.go (1)
go/util/jwt/es256k_test.go (3)
go/node/types/offchain/sign/sign.go (1)
go/util/jwt/es256k.go (1)
go/util/jwt/jwt_test.go (3)
go/util/jwt/es256kadr36.go (4)
🪛 Buf (1.58.0)proto/node/akash/base/offchain/sign/sign.proto4-4: import "gogoproto/gogo.proto": file does not exist (COMPILE) 🪛 Gitleaks (8.28.0)testdata/jwt/cases_es256k.json[high] 4-4: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (jwt) [high] 15-15: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (jwt) [high] 29-29: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (jwt) [high] 53-53: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (jwt) [high] 64-64: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data. (jwt) 🪛 LanguageTooldocs/proto/provider.md[grammar] ~84-~84: There might be a mistake here. (QB_NEW_EN) [grammar] ~85-~85: There might be a mistake here. (QB_NEW_EN) [grammar] ~86-~86: There might be a mistake here. (QB_NEW_EN) [grammar] ~87-~87: There might be a mistake here. (QB_NEW_EN) 🪛 markdownlint-cli2 (0.18.1)docs/proto/provider.md84-84: Unordered list indentation (MD007, ul-indent) 85-85: Unordered list indentation (MD007, ul-indent) 86-86: Unordered list indentation (MD007, ul-indent) 87-87: Unordered list indentation (MD007, ul-indent) 88-88: Unordered list indentation (MD007, ul-indent) 1067-1067: Headings must start at the beginning of the line (MD023, heading-start-left) 1073-1073: Headings must start at the beginning of the line (MD023, heading-start-left) 1080-1080: Link fragments should be valid (MD051, link-fragments) 1090-1090: Headings must start at the beginning of the line (MD023, heading-start-left) 1106-1106: Headings must start at the beginning of the line (MD023, heading-start-left) 1121-1121: Headings must start at the beginning of the line (MD023, heading-start-left) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (26)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
proto/node/akash/base/offchain/sign/sign.proto (1)
16-19: Consider adding size validation for the data field.The
datafield accepts arbitrary bytes without size constraints in the protobuf definition. Consider adding validation logic in the Go implementation to prevent excessively large payloads that could cause memory or performance issues.Would you like me to suggest a validation approach in the Go implementation (sign.go)?
go/node/types/offchain/sign/sign.go (2)
25-32: Add validation for data size limits.As noted in the protobuf review, consider adding a maximum size check for the
Datafield inValidateBasicto prevent excessively large payloads:func (m MsgSignData) ValidateBasic() error { if _, err := sdk.AccAddressFromBech32(m.Signer); err != nil { return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "MsgSignData: Invalid Signer Address") } + + // Prevent excessively large payloads (e.g., 1MB limit) + const maxDataSize = 1 * 1024 * 1024 + if len(m.Data) > maxDataSize { + return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "MsgSignData: Data exceeds maximum size") + } return nil }
34-42: Document panic invariant in GetSigners
Add a comment aboveGetSignersstating that it intentionally panics on invalid Bech32 and relies onValidateBasichaving been called first, mirroring other Cosmos SDK message implementations.go/util/jwt/es256kadr36_test.go (1)
55-60: Fix trivial assertion; compare decoded bytes, not bytes vs stringComparing sig ([]byte) with parts[2] (string) is vacuous. Compare sig with dsig.
- require.NotEqual(s.T(), sig, parts[2]) + require.Equal(s.T(), sig, dsig, "raw signature bytes must match decoded JWT segment")go/util/jwt/es256kadr36.go (2)
45-46: Clarify error for invalid key typeIt expects a SignerI, not specifically a Ledger privkey. Update the message.
- return nil, fmt.Errorf("%w: ES256KADR36 sign expects cryptotypes.LedgerPrivKey", jwt.ErrInvalidKeyType) + return nil, fmt.Errorf("%w: ES256KADR36 Sign expects a jwt.SignerI (keyring.Signer with address)", jwt.ErrInvalidKeyType)
65-66: Unify verify invalid key errorMirror Sign’s phrasing for consistency and clarity.
- return fmt.Errorf("%w: ES256KADR36 verify expects cryptotypes.PubKey", jwt.ErrInvalidKeyType) + return fmt.Errorf("%w: ES256KADR36 Verify expects a jwt.VerifyI (PubKey with address)", jwt.ErrInvalidKeyType)go/util/jwt/signer.go (1)
26-31: Consider narrowing constructor param to the minimal interfaceAccept keyring.Signer instead of keyring.Keyring to reduce coupling.
-func NewSigner(kr keyring.Keyring, addr sdk.Address) SignerI { +func NewSigner(kr keyring.Signer, addr sdk.Address) SignerI {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go/inventory/v1/workloads.pb.gois excluded by!**/*.pb.gogo/node/types/offchain/sign/sign.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
docs/proto/provider.md(2 hunks)go/node/types/offchain/sign/sign.go(1 hunks)go/util/jwt/es256k.go(0 hunks)go/util/jwt/es256k_test.go(1 hunks)go/util/jwt/es256kadr36.go(1 hunks)go/util/jwt/es256kadr36_test.go(1 hunks)go/util/jwt/signer.go(1 hunks)go/util/jwt/suite_test.go(4 hunks)go/util/jwt/verifier.go(1 hunks)proto/node/akash/base/offchain/sign/sign.proto(1 hunks)testdata/jwt/cases_es256kadr36.json(1 hunks)testdata/jwt/jwt.go(1 hunks)
💤 Files with no reviewable changes (1)
- go/util/jwt/es256k.go
🧰 Additional context used
🧬 Code graph analysis (5)
go/util/jwt/signer.go (1)
go/testutil/ids.go (1)
Keyring(18-22)
go/node/types/offchain/sign/sign.go (1)
go/node/types/offchain/sign/sign.pb.go (3)
MsgSignData(27-32)MsgSignData(36-36)MsgSignData(37-39)
go/util/jwt/es256kadr36.go (3)
go/util/jwt/signer.go (2)
SignerI(9-12)Signer(15-18)go/node/types/offchain/sign/sign.pb.go (3)
MsgSignData(27-32)MsgSignData(36-36)MsgSignData(37-39)go/util/jwt/verifier.go (2)
Verify(14-17)VerifyI(9-12)
go/util/jwt/es256kadr36_test.go (5)
go/util/jwt/suite_test.go (1)
IntegrationTestSuite(19-26)testdata/jwt/jwt.go (1)
GetTestsFile(12-30)go/util/jwt/signer.go (1)
Signer(15-18)go/util/jwt/es256kadr36.go (1)
SigningMethodES256KADR36(26-26)go/util/jwt/verifier.go (2)
Verify(14-17)NewVerifier(21-26)
go/util/jwt/es256k_test.go (1)
go/util/jwt/verifier.go (2)
Verify(14-17)NewVerifier(21-26)
🪛 Buf (1.58.0)
proto/node/akash/base/offchain/sign/sign.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🪛 Gitleaks (8.28.0)
testdata/jwt/cases_es256kadr36.json
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
docs/proto/provider.md
84-84: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
85-85: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
86-86: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
88-88: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
1067-1067: Headings must start at the beginning of the line
(MD023, heading-start-left)
1073-1073: Headings must start at the beginning of the line
(MD023, heading-start-left)
1080-1080: Link fragments should be valid
(MD051, link-fragments)
1090-1090: Headings must start at the beginning of the line
(MD023, heading-start-left)
1106-1106: Headings must start at the beginning of the line
(MD023, heading-start-left)
1121-1121: Headings must start at the beginning of the line
(MD023, heading-start-left)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: go
🔇 Additional comments (14)
testdata/jwt/cases_es256kadr36.json (2)
4-4: Static analysis false positive—test JWT token.The Gitleaks warning about a generic API key is a false positive. This is a test JWT token (header.payload.signature format), not a sensitive credential.
1-27: LGTM!The test data structure is well-organized with clear descriptions and expected outcomes for both valid and invalid signature scenarios.
go/util/jwt/es256k_test.go (1)
63-63: LGTM!The update correctly adopts the new verifier abstraction by using
NewVerifier(s.pubKey, s.addr)instead of directly passing the public key. This aligns with the architectural change to introduce explicit verifier objects.testdata/jwt/jwt.go (1)
9-9: LGTM!Correctly adds the new ES256KADR36 test data file to the embedded filesystem, making it accessible via
GetTestsFile.go/util/jwt/suite_test.go (2)
10-10: LGTM!The addition of
addrandpubKeyfields to the test suite correctly supports the new verifier abstraction, enabling tests to construct verifier objects viaNewVerifier(pubKey, addr).Also applies to: 24-25, 50-52
63-63: Consider enabling the ES256KADR36 test suite.The ES256KADR36Test suite is commented out. If the implementation is ready, consider enabling it to ensure test coverage. If tests are unstable or incomplete, document the reason for disabling them.
Would you like me to help verify whether the ES256KADR36Test is ready to be enabled by examining the test implementation?
proto/node/akash/base/offchain/sign/sign.proto (1)
4-4: Static analysis false positive—gogoproto is a standard dependency.The Buf warning about the missing
gogoproto/gogo.protoimport is a false positive. In Cosmos SDK projects, gogoproto is a standard, well-known dependency that's available during build. This import is valid.go/util/jwt/verifier.go (1)
1-34: LGTM! Clean verifier abstraction.This verifier abstraction is well-designed:
- Clear interface with minimal surface area
- Proper embedding of
cryptotypes.PubKeyfor delegation- Compile-time interface compliance check
- Simple, focused constructor
The abstraction provides a clean separation between verification concerns and key management.
docs/proto/provider.md (1)
1-1: Auto-generated docs—fix formatting in generator template
The markdown issues (indentation, heading alignment, link fragments) originate in the generator’s template (docs/protodoc-markdown.tmpl, used byscript/protocgen-legacy.sh). Apply formatting fixes there so they propagate intoprovider.mdrather than editing the generated file.go/util/jwt/es256kadr36.go (2)
29-44: ES256KADR36 Sign flow looks correctStdSignBytes + MsgSignData + SignByAddress aligns with ADR-36 off-chain signing. No blocking issues.
73-79: Registration of custom signing method LGTMAlg() string and RegisterSigningMethod are correct; global var exposure is fine.
go/util/jwt/signer.go (1)
14-24: SignerI/Signer implementation is cleanEmbedded keyring.Signer + GetAddress() is straightforward and fits the ES256KADR36 flow.
go/util/jwt/es256kadr36_test.go (2)
50-53: Helpers encodeSegment/decodeSegment are defined
The functions are implemented in es256k_test.go under the same package jwt, so they’re available in es256kadr36_test.go. No action needed.
46-48: Guard against nil signing methodInsert a nil check after
jwt.GetSigningMethodto prevent panics:method := jwt.GetSigningMethod(tc.Expected.Alg) + require.NotNil(s.T(), method, "unknown signing method: %s", tc.Expected.Alg) sig, err := method.Sign(toSign, key)Verify that your test fixtures don’t contain unknown
algvalues that could makeGetSigningMethodreturn nil.
| var ( | ||
| MsgTypeSignData = "" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Initialize MsgTypeSignData directly instead of using reflection.
The empty string initialization followed by reflection in init() is unnecessarily complex. Consider initializing directly:
-var (
- MsgTypeSignData = ""
-)
+const MsgTypeSignData = "MsgSignData"This is clearer, performs better (compile-time constant vs. runtime reflection), and eliminates the dependency on init().
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In go/node/types/offchain/sign/sign.go around lines 10 to 12, replace the empty
string plus runtime reflection pattern by initializing MsgTypeSignData directly
as a constant string (e.g. set MsgTypeSignData = "sign/MsgSignData" or the exact
type URL/name you intend to use), remove the now-unnecessary init() reflection
code, and make MsgTypeSignData a package-level const or var initialized at
declaration so the value is available at compile time.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
go/node/types/offchain/sign/sign.go (1)
14-16: Make MsgTypeSignData a constant; avoid reflectionInitialize directly (compile-time) instead of via reflect in init.
-var ( - MsgTypeSignData = "" -) +const MsgTypeSignData = "MsgSignData" - -func init() { - MsgTypeSignData = reflect.TypeOf(&MsgSignData{}).Elem().Name() -}Also applies to: 23-25
🧹 Nitpick comments (5)
go/util/jwt/verifier.go (1)
8-12: Minor naming consistencyConsider renaming Pubkey() → PubKey() for consistency with Cosmos naming.
testdata/jwt/cases_es256k.json (1)
4-4: Silence Gitleaks for test JWT fixturesThese tokens are test fixtures, not secrets. Add a Gitleaks allowlist for testdata/jwt/** to prevent false positives in CI.
Example gitleaks.toml rule:
[[rules]] description = "Allow test JWTs" id = "allow-test-jwts" regex = '''eyJ[a-zA-Z0-9_-]+?\.[a-zA-Z0-9_-]+?\.[a-zA-Z0-9_-]+?''' path = '''^testdata/jwt/''' allowlist = { paths = ["testdata/jwt/"] }Also applies to: 15-15, 28-28
go/util/jwt/es256k.go (1)
41-43: Tighten invalid key error messageError still says “expects cryptotypes.PubKey” but Verify now expects VerifyI. Update message for clarity.
- return fmt.Errorf("%w: ES256K verify expects cryptotypes.PubKey", jwt.ErrInvalidKeyType) + return fmt.Errorf("%w: ES256K verify expects VerifyI", jwt.ErrInvalidKeyType)go/util/jwt/es256kadr36.go (1)
46-47: Clarify invalid key type errorsAlign error messages with expected interfaces (SignerI / VerifyI).
- return nil, fmt.Errorf("%w: ES256KADR36 sign expects cryptotypes.LedgerPrivKey", jwt.ErrInvalidKeyType) + return nil, fmt.Errorf("%w: ES256KADR36 sign expects SignerI", jwt.ErrInvalidKeyType) @@ - return fmt.Errorf("%w: ES256KADR36 verify expects cryptotypes.PubKey", jwt.ErrInvalidKeyType) + return fmt.Errorf("%w: ES256KADR36 verify expects VerifyI", jwt.ErrInvalidKeyType)Also applies to: 66-67
go/util/jwt/signer.go (1)
26-31: Accept narrower dependency in constructorNewSigner only needs signing; take keyring.Signer instead of keyring.Keyring to reduce coupling and improve testability.
-func NewSigner(kr keyring.Keyring, addr sdk.Address) SignerI { +func NewSigner(kr keyring.Signer, addr sdk.Address) SignerI { return &Signer{ Signer: kr, addr: addr, } }Optional nits:
- Consider renaming the struct field Signer to signer or backend to avoid confusion with the type name Signer.
- Go style prefers Address() over GetAddress(); consider aligning if API is still fluid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go/inventory/v1/workloads.pb.gois excluded by!**/*.pb.gogo/node/types/offchain/sign/sign.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
docs/proto/provider.md(2 hunks)go/node/types/offchain/sign/sign.go(1 hunks)go/util/jwt/es256k.go(1 hunks)go/util/jwt/es256k_test.go(2 hunks)go/util/jwt/es256kadr36.go(1 hunks)go/util/jwt/jwt_test.go(6 hunks)go/util/jwt/signer.go(1 hunks)go/util/jwt/suite_test.go(3 hunks)go/util/jwt/verifier.go(1 hunks)proto/node/akash/base/offchain/sign/sign.proto(1 hunks)testdata/jwt/cases_es256k.json(3 hunks)testdata/jwt/cases_jwt.json.tmpl(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- testdata/jwt/cases_jwt.json.tmpl
🚧 Files skipped from review as they are similar to previous changes (1)
- go/util/jwt/suite_test.go
🧰 Additional context used
🧬 Code graph analysis (6)
go/util/jwt/es256k.go (1)
go/util/jwt/verifier.go (1)
VerifyI(9-12)
go/util/jwt/es256k_test.go (2)
go/util/jwt/signer.go (1)
NewSigner(26-31)go/util/jwt/verifier.go (2)
NewVerifier(21-26)Verify(14-17)
go/util/jwt/jwt_test.go (3)
go/util/jwt/jwt.go (1)
Claims(90-96)go/util/jwt/signer.go (2)
Signer(15-18)NewSigner(26-31)go/util/jwt/verifier.go (1)
NewVerifier(21-26)
go/util/jwt/es256kadr36.go (4)
go/util/jwt/signer.go (2)
SignerI(9-12)Signer(15-18)go/node/types/offchain/sign/sign.go (1)
StdSignBytes(61-86)go/node/types/offchain/sign/sign.pb.go (3)
MsgSignData(27-32)MsgSignData(36-36)MsgSignData(37-39)go/util/jwt/verifier.go (2)
Verify(14-17)VerifyI(9-12)
go/util/jwt/signer.go (1)
go/testutil/ids.go (1)
Keyring(18-22)
go/node/types/offchain/sign/sign.go (1)
go/node/types/offchain/sign/sign.pb.go (3)
MsgSignData(27-32)MsgSignData(36-36)MsgSignData(37-39)
🪛 Buf (1.58.0)
proto/node/akash/base/offchain/sign/sign.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🪛 Gitleaks (8.28.0)
testdata/jwt/cases_es256k.json
[high] 4-4: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 15-15: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 28-28: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🪛 LanguageTool
docs/proto/provider.md
[grammar] ~84-~84: There might be a mistake here.
Context: ... - akash/inventory/v1/workloads.proto - Workload ...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ...ory/v1/workloads.proto) - Workload - [WorkloadReplica](#akash.inventory.v1.Wor...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...ory.v1.Workload) - WorkloadReplica - [WorkloadService](#akash.inventory.v1.Wor...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...WorkloadReplica) - WorkloadService - [WorkloadStatus](#akash.inventory.v1.Work...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
docs/proto/provider.md
84-84: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
85-85: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
86-86: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
88-88: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
1067-1067: Headings must start at the beginning of the line
(MD023, heading-start-left)
1073-1073: Headings must start at the beginning of the line
(MD023, heading-start-left)
1080-1080: Link fragments should be valid
(MD051, link-fragments)
1090-1090: Headings must start at the beginning of the line
(MD023, heading-start-left)
1106-1106: Headings must start at the beginning of the line
(MD023, heading-start-left)
1121-1121: Headings must start at the beginning of the line
(MD023, heading-start-left)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coverage
- GitHub Check: go
🔇 Additional comments (2)
go/util/jwt/es256k_test.go (1)
58-76: Review comment incorrectly assumes string-based API
In jwt/v5, SigningMethod.Sign returns ([]byte, error) and Verify accepts []byte, so encodeSegment/decodeSegment usage is correct.Likely an incorrect or invalid review comment.
go/util/jwt/jwt_test.go (1)
62-87: Do not remove encodeSegment; Sign returns raw bytes
SigningMethod.Sign returns []byte, so wrapping its output in encodeSegment(sig) to Base64URL-encode is correct—treating it as a pre-encoded string is invalid.Likely an incorrect or invalid review comment.
| // GetSignBytes encodes the message for signing | ||
| func (m *MsgSignData) GetSignBytes() []byte { | ||
| return sdk.MustSortJSON(legacy.Cdc.MustMarshalJSON(m)) | ||
| } |
There was a problem hiding this comment.
Register MsgSignData with the legacy Amino codec (ADR‑36 compliance)
StdSignBytes/GetSignBytes rely on legacy.Cdc JSON. Without RegisterConcrete for MsgSignData under the expected name (commonly "sign/MsgSignData"), wallets (Keplr/Leap) won’t produce/verify ADR‑36 JSON consistently.
Add a legacy codec registration in this package (init or a RegisterLegacyAminoCodec function):
+import "github.com/cosmos/cosmos-sdk/codec"
+
+func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
+ cdc.RegisterConcrete(&MsgSignData{}, "sign/MsgSignData", nil)
+}
+
+func init() {
+ // Optional: also register on the global legacy codec if used in this lib
+ legacy.Cdc.RegisterConcrete(&MsgSignData{}, "sign/MsgSignData", nil)
+}Then ensure your app/init calls RegisterLegacyAminoCodec on its codec. Based on learnings
Also applies to: 60-86
🤖 Prompt for AI Agents
In go/node/types/offchain/sign/sign.go around lines 51-54 (and similarly for
lines 60-86), MsgSignData is marshaled with legacy.Cdc but is not registered
with the legacy Amino codec; add a RegisterLegacyAminoCodec function (or an
init) in this package that calls legacy.Cdc.RegisterConcrete(&MsgSignData{},
"sign/MsgSignData", nil) (use the exact canonical name "sign/MsgSignData"),
export the function if you choose a RegisterLegacyAminoCodec pattern and ensure
the application init code calls it so wallets (Keplr/Leap) produce/verify ADR-36
JSON consistently.
| syntax = "proto3"; | ||
| package akash.base.offchain.sign; | ||
|
|
||
| import "gogoproto/gogo.proto"; |
There was a problem hiding this comment.
🧩 Analysis chain
Fix buf proto dependency for gogoproto
Buf cannot resolve gogoproto/gogo.proto. Add the buf dep (e.g., cosmos/gogo) or vendor the file so codegen doesn’t fail.
If you’re using buf, add to buf.yaml:
- deps: ["buf.build/cosmos/gogo"]
Or ensure your buf.work/buf.gen.yaml pull in the gogo module.
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Buf config files ==="
find . -type f \( -name 'buf.yaml' -o -name 'buf.gen.yaml' -o -name 'buf.work.yaml' \)
echo -e "\n=== Snippet from sign.proto ==="
sed -n '1,20p' proto/node/akash/base/offchain/sign/sign.proto || echo "File not found"
echo -e "\n=== Searching for gogo.proto files ==="
find . -type f -name 'gogo.proto' || echo "No gogo.proto files found"
echo -e "\n=== Searching for gogoproto directories ==="
find . -type d -name 'gogoproto' || echo "No gogoproto directories found"
echo -e "\n=== Searching for 'cosmos/gogo' in Buf configs ==="
grep -R --include 'buf*.yaml' "cosmos/gogo" -n . || echo "No 'cosmos/gogo' references in Buf configs"Length of output: 1042
Fix Buf config for gogoproto
Uncomment or add - buf.build/cosmos/gogo-proto under deps in proto/node/buf.yaml (and proto/provider/buf.yaml), or vendor gogoproto/gogo.proto locally so Buf can resolve the import.
🧰 Tools
🪛 Buf (1.58.0)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🤖 Prompt for AI Agents
In proto/node/akash/base/offchain/sign/sign.proto around line 4, the import
"gogoproto/gogo.proto" cannot be resolved by Buf; update proto/node/buf.yaml and
proto/provider/buf.yaml to include the gogoproto dependency by adding or
uncommenting "- buf.build/cosmos/gogo-proto" under deps, then run buf mod
update, or alternatively vendor the gogoproto files into the repo (e.g., add
gogoproto/gogo.proto under a proto/vendor/gogoproto path and ensure the buf.yaml
`paths`/module configuration includes that vendor directory) so the import can
be resolved.
https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-036-arbitrary-signature.md Signed-off-by: Artur Troian <troian@users.noreply.github.com>
https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-036-arbitrary-signature.md
🌟 PR Title
[Use a clear, concise title that reflects the change]
📝 Description
[Explain what this PR does in 2-3 sentences. Include context about the feature or problem being solved]
🔧 Purpose of the Change
📌 Related Issues
✅ Checklist
📎 Notes for Reviewers
[Include any additional context, architectural decisions, or specific areas to focus on]
Summary by CodeRabbit
New Features
Documentation
Tests