-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RelayMiner] Fix LeanClient mutiple suppliers support #662
Conversation
WalkthroughThe modifications introduce new interfaces for claim and proof messages, update the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- pkg/client/interface.go (5 hunks)
- pkg/client/supplier/client.go (4 hunks)
- pkg/client/supplier/client_test.go (3 hunks)
- pkg/relayer/proxy/server_builder.go (2 hunks)
- pkg/relayer/proxy/synchronous.go (2 hunks)
- pkg/relayer/session/claim.go (6 hunks)
- pkg/relayer/session/proof.go (7 hunks)
- pkg/relayer/session/session.go (13 hunks)
- pkg/relayer/session/sessiontree.go (4 hunks)
- pkg/relayer/types.go (2 hunks)
- testutil/testclient/testsupplier/client.go (2 hunks)
- x/proof/keeper/msg_server_submit_proof_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- pkg/relayer/types.go
Additional comments not posted (44)
pkg/client/supplier/client.go (6)
5-5
: Add import for sync package.The import for the
sync
package is necessary for thesync.Mutex
field added to thesupplierClient
struct.
23-24
: Add pendingTxMu to supplierClient struct.The
pendingTxMu
field is added to thesupplierClient
struct to prevent concurrent transactions with the same sequence number. This is a good practice to ensure thread safety.
69-72
: Lock and unlock pendingTxMu in SubmitProofs.The
pendingTxMu
is locked at the beginning of theSubmitProofs
method and unlocked usingdefer
to ensure that it is always unlocked, even if an error occurs. This is a good practice to prevent deadlocks.
113-117
: Lock and unlock pendingTxMu in CreateClaims.The
pendingTxMu
is locked at the beginning of theCreateClaims
method and unlocked usingdefer
to ensure that it is always unlocked, even if an error occurs. This is a good practice to prevent deadlocks.
88-93
: Ensure correct type casting in SubmitProofs.The type casting of
proofMsgs
to*prooftypes.MsgSubmitProof
is done without checking. Ensure thatclient.MsgSubmitProof
is always of type*prooftypes.MsgSubmitProof
to prevent runtime errors.Verification successful
Type casting is verified to be correct.
The type casting of
proofMsgs
to*prooftypes.MsgSubmitProof
is safe becauseproofMsgs
is populated with elements of type*prooftypes.MsgSubmitProof
.
pkg/relayer/session/proof.go
:proofMsgs
is initialized and appended with&prooftypes.MsgSubmitProof
.pkg/client/supplier/client.go
: The type casting occurs here and is verified to be correct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that client.MsgSubmitProof is always of type *prooftypes.MsgSubmitProof. # Test: Search for the type definition. Expect: client.MsgSubmitProof is of type *prooftypes.MsgSubmitProof. ast-grep --lang go --pattern $'type MsgSubmitProof interface { GetProof() []byte GetSessionHeader() *sessiontypes.SessionHeader GetSupplierAddress() string }'Length of output: 165
Script:
#!/bin/bash # Description: Verify the type of proofMsgs and ensure it is always of type *prooftypes.MsgSubmitProof. # Find the definition and initialization of proofMsgs rg 'proofMsgs' -A 5 # Search for the usage of prooftypes.MsgSubmitProof to understand its type and usage rg 'prooftypes.MsgSubmitProof'Length of output: 5093
134-139
: Ensure correct type casting in CreateClaims.The type casting of
claimMsgs
to*prooftypes.MsgCreateClaim
is done without checking. Ensure thatclient.MsgCreateClaim
is always of type*prooftypes.MsgCreateClaim
to prevent runtime errors.Verification successful
Type casting in CreateClaims is correct and safe.
The
claimMsgs
slice is consistently populated with elements of type*prooftypes.MsgCreateClaim
, ensuring that the type casting inpkg/client/supplier/client.go
does not pose any runtime risks.
pkg/relayer/session/claim.go
:claimMsgs
is initialized and populated with&prooftypes.MsgCreateClaim
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that client.MsgCreateClaim is always of type *prooftypes.MsgCreateClaim. # Test: Search for the type definition. Expect: client.MsgCreateClaim is of type *prooftypes.MsgCreateClaim. ast-grep --lang go --pattern $'type MsgCreateClaim interface { GetRootHash() []byte GetSessionHeader() *sessiontypes.SessionHeader GetSupplierAddress() string }'Length of output: 168
Script:
#!/bin/bash # Search for references and definitions of MsgCreateClaim in the codebase rg 'MsgCreateClaim'Length of output: 29910
Script:
#!/bin/bash # Search for the definition and initialization of claimMsgs in the codebase rg 'claimMsgs'Length of output: 609
pkg/client/supplier/client_test.go (1)
20-20
: Update imports to use prooftypes and sessiontypes packages.The imports have been updated to use the
prooftypes
andsessiontypes
packages. Ensure that these changes are consistent with the rest of the codebase.pkg/relayer/proxy/server_builder.go (2)
100-102
: Remove supplierServices parameter from initializeProxyServers.The
supplierServices
parameter has been removed from theinitializeProxyServers
method. Ensure that this change is consistent with the rest of the codebase and that thesupplierServices
parameter is no longer needed.
109-109
: Update initializeProxyServers method.The
initializeProxyServers
method has been updated to no longer require thesupplierServices
parameter. Ensure that this change is consistent with the rest of the codebase and that thesupplierServices
parameter is no longer needed.pkg/relayer/session/sessiontree.go (1)
230-230
: Ensure proper synchronization when accessingisClaiming
.Setting
isClaiming
tofalse
without checking its current state or ensuring thread safety might lead to race conditions or inconsistent states.Ensure that all accesses to
isClaiming
are properly synchronized.pkg/relayer/session/claim.go (10)
14-14
: Importprooftypes
correctly.The
prooftypes
import is necessary for the updatedcreateClaims
method.
26-27
: PasssupplierClient
tocreateClaims
.The
createClaims
method now requiressupplierClient
as a parameter, which is necessary for creating claims.
35-35
: Ensure proper error handling formapWaitForEarliestCreateClaimsHeight
.Ensure that any errors encountered during the mapping process are properly handled to prevent unexpected behavior.
Check that errors are handled correctly in the mapping function.
44-44
: PasssupplierClient
tonewMapClaimSessionsFn
.The
newMapClaimSessionsFn
function now requiressupplierClient
as a parameter, which is necessary for creating claims.
54-58
: Delete expired session trees.The code deletes expired session trees to prevent them from being claimed again.
111-111
: Log errors when failing to get shared params.Ensure that errors encountered when getting shared params are logged correctly.
163-163
: PasssupplierClient
tonewMapClaimSessionsFn
.The
newMapClaimSessionsFn
function now requiressupplierClient
as a parameter, which is necessary for creating claims.
175-180
: Create claim messages for each session tree.The code creates claim messages for each session tree using the updated
MsgCreateClaim
interface.
185-188
: Handle errors when creating claims.Ensure that any errors encountered when creating claims are properly handled and logged.
214-214
: Log errors when failing to flush session.Ensure that errors encountered when flushing sessions are logged correctly.
pkg/relayer/session/proof.go (10)
15-15
: Importprooftypes
correctly.The
prooftypes
import is necessary for the updatedsubmitProofs
method.
27-27
: PasssupplierClient
tosubmitProofs
.The
submitProofs
method now requiressupplierClient
as a parameter, which is necessary for submitting proofs.
45-45
: PasssupplierClient
tonewMapProveSessionsFn
.The
newMapProveSessionsFn
function now requiressupplierClient
as a parameter, which is necessary for submitting proofs.
52-56
: Delete expired session trees.The code deletes expired session trees to prevent them from being proven again.
99-99
: Log errors when failing to get shared params.Ensure that errors encountered when getting shared params are logged correctly.
165-165
: PasssupplierClient
tonewMapProveSessionsFn
.The
newMapProveSessionsFn
function now requiressupplierClient
as a parameter, which is necessary for submitting proofs.
177-182
: Create proof messages for each session tree.The code creates proof messages for each session tree using the updated
MsgSubmitProof
interface.
187-190
: Handle errors when submitting proofs.Ensure that any errors encountered when submitting proofs are properly handled and logged.
194-198
: Remove session tree from relayer sessions and delete it.The code removes the session tree from relayer sessions and deletes it after submitting proofs.
235-235
: Log errors when failing to generate proof.Ensure that errors encountered when generating proofs are logged correctly.
pkg/client/interface.go (4)
35-42
: UpdateMsgCreateClaim
interface.The
MsgCreateClaim
interface has been updated to include methods for retrieving root hash, session header, and supplier address.
44-51
: UpdateMsgSubmitProof
interface.The
MsgSubmitProof
interface has been updated to include methods for retrieving proof, session header, and supplier address.
Line range hint
62-71
:
UpdateSupplierClient
interface.The
SupplierClient
interface methodsCreateClaims
andSubmitProofs
have been updated to use the newMsgCreateClaim
andMsgSubmitProof
interfaces.
330-330
: UpdateBlockQueryClient
interface.The
Block
method now returnscometrpctypes.ResultBlock
instead ofcoretypes.ResultBlock
.pkg/relayer/proxy/synchronous.go (2)
Line range hint
26-36
:
Verify the removal ofsupplierServiceMap
parameter across the codebase.Ensure that all calls to
NewSynchronousServer
have been updated to remove thesupplierServiceMap
parameter.Verification successful
The removal of the
supplierServiceMap
parameter from theNewSynchronousServer
function has been verified.
- The
NewSynchronousServer
call inpkg/relayer/proxy/server_builder.go
does not include thesupplierServiceMap
parameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `NewSynchronousServer` have been updated to remove the `supplierServiceMap` parameter. # Test: Search for the function usage. Expect: No occurrences with the old parameter. rg --type go $'NewSynchronousServer'Length of output: 303
Script:
#!/bin/bash # Description: Inspect the `NewSynchronousServer` call in `pkg/relayer/proxy/server_builder.go` to ensure the `supplierServiceMap` parameter is not present. # Extract the lines around the `NewSynchronousServer` call to inspect its parameters. rg -A 5 -B 5 --type go $'NewSynchronousServer' pkg/relayer/proxy/server_builder.goLength of output: 494
Line range hint
11-23
:
Verify the removal ofsupplierServiceMap
field across the codebase.Ensure that all references to
supplierServiceMap
have been removed or updated accordingly in the codebase.Verification successful
Verify the removal of
supplierServiceMap
field across the codebase.All references to
supplierServiceMap
have been removed or updated accordingly in the codebase.
- No occurrences of
supplierServiceMap
found in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `supplierServiceMap` have been removed or updated. # Test: Search for the field usage. Expect: No occurrences. rg --type go $'supplierServiceMap'Length of output: 33
Script:
#!/bin/bash # Description: Verify all references to `supplierServiceMap` have been removed or updated. # Test: Search for the field usage. Expect: No occurrences. rg 'supplierServiceMap'Length of output: 23
Script:
#!/bin/bash # Description: Verify all references to `supplierServiceMap` have been removed or updated. # Test: Search for the field usage across all files. Expect: No occurrences. rg 'supplierServiceMap'Length of output: 23
pkg/relayer/session/session.go (7)
145-190
: Verify the addition ofsupplierAddress
inensureSessionTree
.Ensure that all references to
supplierAddress
in theensureSessionTree
function are correct and consistent.Verification successful
The addition of
supplierAddress
in theensureSessionTree
function is correct and consistent.The
supplierAddress
is referenced correctly within theensureSessionTree
function and throughout thepkg/relayer/session/session.go
file.
pkg/relayer/session/session.go
: Verified references tosupplierAddress
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the addition of `supplierAddress` in the `ensureSessionTree` function. # Test: Search for the function usage. Expect: Correct and consistent references. rg --type go $'supplierAddress'Length of output: 10390
116-127
: Verify the addition ofsupplierClient
parameters.Ensure that all references to
supplierClient
in theStart
function are correct and consistent.Verification successful
The addition of
supplierClient
parameters has been verified and is consistent throughout the codebase.
pkg/relayer/session/claim.go
pkg/relayer/session/session.go
pkg/relayer/session/proof.go
- Various test files
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the addition of the `supplierClient` parameters. # Test: Search for the function usage. Expect: Correct and consistent references. rg --type go $'supplierClient'Length of output: 4728
Line range hint
54-64
:
Verify the initialization of thesessionsTrees
map.Ensure that the
sessionsTrees
map is correctly initialized with the new structure.Verification successful
The initialization of the
sessionsTrees
map is correct.The
sessionsTrees
map is defined assessionsTreesMap
and is properly initialized using themake
function in thesession.go
file.
sessionsTrees
definition:type sessionsTreesMap = map[int64]map[string]map[string]relayer.SessionTree
sessionsTrees
initialization:sessionsTrees: make(sessionsTreesMap)
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of the `sessionsTrees` map. # Test: Search for the initialization. Expect: Correct structure. rg --type go $'make(sessionsTreesMap)'Length of output: 39
Script:
#!/bin/bash # Search for the definition of the sessionsTrees map in the session.go file rg --type go 'sessionsTrees' pkg/relayer/session/session.go # Search for the initialization of the sessionsTrees map in the session.go file rg --type go 'sessionsTrees\s*=\s*make' pkg/relayer/session/session.goLength of output: 1894
24-28
: Verify the changes to thesessionsTrees
map.Ensure that all references to the
sessionsTrees
map have been updated to includesupplierAddress
.Verification successful
Verification Successful: All references to the
sessionsTrees
map have been correctly updated to includesupplierAddress
.The changes to the
sessionsTrees
map are consistent and correctly implemented.
ensureSessionTree
: Initializes nested maps includingsupplierAddress
.forEachBlockClaimSessionsFn
: Iterates oversupplierSessionTrees
and accessessessionsSupplier
.removeFromRelayerSessions
: Accesses and deletes entries usingsupplierAddress
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the `sessionsTrees` map have been updated to include `supplierAddress`. # Test: Search for the map usage. Expect: All occurrences include `supplierAddress`. rg --type go $'sessionsTrees'Length of output: 2658
Script:
#!/bin/bash # Extract and display lines around references to `sessionsTrees` to check for correct usage of `supplierAddress`. # Display lines around the references to `sessionsTrees` rg --type go --context 5 'sessionsTrees' pkg/relayer/session/session.goLength of output: 8069
307-345
: Verify the addition ofsupplierAddress
inremoveFromRelayerSessions
.Ensure that all references to
supplierAddress
in theremoveFromRelayerSessions
function are correct and consistent.Verification successful
The addition of
supplierAddress
inremoveFromRelayerSessions
is correct and consistent.The
supplierAddress
is used appropriately within the function to manage session trees and log relevant information.
pkg/relayer/session/session.go
:
- Line 312:
supplierAddress
is extracted fromsessionTree
.- Lines 330, 332, 338:
supplierAddress
is used for logging and managing thesuppliersSessionTrees
map.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the addition of `supplierAddress` in the `removeFromRelayerSessions` function. # Test: Search for the function usage. Expect: Correct and consistent references. rg --type go $'supplierAddress'Length of output: 10390
Script:
#!/bin/bash # Extract the entire removeFromRelayerSessions function from pkg/relayer/session/session.go ast-grep --lang go --pattern $'func (rs *relayerSessionsManager) removeFromRelayerSessions($_) { $$$ }' pkg/relayer/session/session.goLength of output: 3206
412-444
: Verify the implementation ofdeleteExpiredSessionTreesFn
.Ensure that the implementation of the
deleteExpiredSessionTreesFn
function is correct and consistent.Verification successful
The implementation of
deleteExpiredSessionTreesFn
is correct and consistent.The function is used appropriately in both
pkg/relayer/session/claim.go
andpkg/relayer/session/proof.go
, with the correct parameters being passed.
pkg/relayer/session/claim.go
:shared.GetClaimWindowCloseHeight
pkg/relayer/session/proof.go
:shared.GetProofWindowCloseHeight
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `deleteExpiredSessionTreesFn` function. # Test: Search for the function usage. Expect: Correct and consistent implementation. rg --type go $'deleteExpiredSessionTreesFn'Length of output: 330
Script:
#!/bin/bash # Extract the relevant code snippets from claim.go and proof.go to review the function's usage. # Extract the usage in claim.go rg -A 10 -B 10 'deleteExpiredSessionTreesFn' pkg/relayer/session/claim.go # Extract the usage in proof.go rg -A 10 -B 10 'deleteExpiredSessionTreesFn' pkg/relayer/session/proof.goLength of output: 2110
Line range hint
217-285
:
Verify the addition ofsessionsSupplier
inforEachBlockClaimSessionsFn
.Ensure that all references to
sessionsSupplier
in theforEachBlockClaimSessionsFn
function are correct and consistent.x/proof/keeper/msg_server_submit_proof_test.go (1)
Line range hint
15-15
: LGTM! The callback parameter has been removed fromnewEmptySessionTree
.The function signature is simplified by removing the unused callback parameter.
@@ -60,15 +59,15 @@ func NewOneTimeClaimProofSupplierClientMap( | |||
supplierClientMock.EXPECT(). | |||
CreateClaims( | |||
gomock.Eq(ctx), | |||
gomock.AssignableToTypeOf([]*relayer.SessionClaim{}), | |||
gomock.AssignableToTypeOf(([]client.MsgCreateClaim)(nil)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Ensure correct type assertion for CreateClaims.
The client.MsgCreateClaim
type does not implement the necessary methods (GetRootHash
, GetSessionHeader
, GetSupplierAddress
). Therefore, the type assertion for CreateClaims
using client.MsgCreateClaim
is incorrect and will cause issues.
MsgCreateClaim
definition found in:./x/proof/types/tx.pb.go
./api/poktroll/proof/tx.pulsar.go
- No methods
GetRootHash
,GetSessionHeader
,GetSupplierAddress
found forMsgCreateClaim
.
Analysis chain
Ensure correct type assertion for CreateClaims.
The type assertion for CreateClaims
has been updated to use client.MsgCreateClaim
. Ensure that this change is consistent with the rest of the codebase and that client.MsgCreateClaim
implements the necessary interface.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that client.MsgCreateClaim implements the necessary interface.
# Test: Search for the interface implementation. Expect: client.MsgCreateClaim implements the required interface.
ast-grep --lang go --pattern $'type MsgCreateClaim interface {
GetRootHash() []byte
GetSessionHeader() *sessiontypes.SessionHeader
GetSupplierAddress() string
}'
Length of output: 168
Script:
#!/bin/bash
# Description: Verify that client.MsgCreateClaim implements the necessary methods.
# Search for the GetRootHash method in client.MsgCreateClaim
ast-grep --lang go --pattern $'func ($_ client.MsgCreateClaim) GetRootHash() []byte {
$$$
}'
# Search for the GetSessionHeader method in client.MsgCreateClaim
ast-grep --lang go --pattern $'func ($_ client.MsgCreateClaim) GetSessionHeader() *sessiontypes.SessionHeader {
$$$
}'
# Search for the GetSupplierAddress method in client.MsgCreateClaim
ast-grep --lang go --pattern $'func ($_ client.MsgCreateClaim) GetSupplierAddress() string {
$$$
}'
Length of output: 320
Script:
#!/bin/bash
# Description: Find the definition of client.MsgCreateClaim and list its methods.
# Search for the definition of client.MsgCreateClaim
ast-grep --lang go --pattern $'type MsgCreateClaim struct {
$$$
}'
# List methods of client.MsgCreateClaim
ast-grep --lang go --pattern $'func ($_ MsgCreateClaim) $_($_) $_ {
$$$
}'
Length of output: 1756
). | ||
Return(nil). | ||
Times(1) | ||
|
||
supplierClientMock.EXPECT(). | ||
SubmitProofs( | ||
gomock.Eq(ctx), | ||
gomock.AssignableToTypeOf([]*relayer.SessionProof{}), | ||
gomock.AssignableToTypeOf(([]client.MsgSubmitProof)(nil)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Ensure correct type assertion for SubmitProofs.
The type assertion for SubmitProofs
has been updated to use client.MsgSubmitProof
. However, client.MsgSubmitProof
does not implement the necessary interface methods (GetProof() []byte
, GetSessionHeader() *sessiontypes.SessionHeader
, and GetSupplierAddress() string
). Please ensure that client.MsgSubmitProof
implements these methods.
- Files checked:
./x/proof/types/tx.pb.go
./api/poktroll/proof/tx.pulsar.go
Analysis chain
Ensure correct type assertion for SubmitProofs.
The type assertion for SubmitProofs
has been updated to use client.MsgSubmitProof
. Ensure that this change is consistent with the rest of the codebase and that client.MsgSubmitProof
implements the necessary interface.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that client.MsgSubmitProof implements the necessary interface.
# Test: Search for the interface implementation. Expect: client.MsgSubmitProof implements the required interface.
ast-grep --lang go --pattern $'type MsgSubmitProof interface {
GetProof() []byte
GetSessionHeader() *sessiontypes.SessionHeader
GetSupplierAddress() string
}'
Length of output: 165
Script:
#!/bin/bash
# Description: Find the definition of MsgSubmitProof and check if it implements the required methods.
# Step 1: Find the definition of MsgSubmitProof
msg_submit_proof_definition=$(ast-grep --lang go --pattern 'type MsgSubmitProof struct { $$$ }')
# Step 2: Check if MsgSubmitProof implements the required methods
if [ -n "$msg_submit_proof_definition" ]; then
# Extract the file path of MsgSubmitProof definition
msg_submit_proof_file=$(echo "$msg_submit_proof_definition" | grep -oP '^\S+')
# Search for the required methods in the MsgSubmitProof file
ast-grep --lang go --pattern 'func (m MsgSubmitProof) GetProof() []byte' "$msg_submit_proof_file"
ast-grep --lang go --pattern 'func (m MsgSubmitProof) GetSessionHeader() *sessiontypes.SessionHeader' "$msg_submit_proof_file"
ast-grep --lang go --pattern 'func (m MsgSubmitProof) GetSupplierAddress() string' "$msg_submit_proof_file"
else
echo "MsgSubmitProof definition not found."
fi
Length of output: 9490
Script:
#!/bin/bash
# Description: Correctly extract file paths and verify that MsgSubmitProof implements the required methods.
# Step 1: Find the definition of MsgSubmitProof
msg_submit_proof_definition=$(ast-grep --lang go --pattern 'type MsgSubmitProof struct { $$$ }')
# Step 2: Extract file paths without line numbers
msg_submit_proof_files=$(echo "$msg_submit_proof_definition" | grep -oP '^\S+' | uniq)
# Step 3: Check if MsgSubmitProof implements the required methods
for file in $msg_submit_proof_files; do
echo "Checking file: $file"
ast-grep --lang go --pattern 'func (m MsgSubmitProof) GetProof() []byte' "$file"
ast-grep --lang go --pattern 'func (m MsgSubmitProof) GetSessionHeader() *sessiontypes.SessionHeader' "$file"
ast-grep --lang go --pattern 'func (m MsgSubmitProof) GetSupplierAddress() string' "$file"
done
Length of output: 16384
Script:
#!/bin/bash
# Description: Correctly extract file paths and verify that MsgSubmitProof implements the required methods.
# Step 1: Find the definition of MsgSubmitProof
msg_submit_proof_definition=$(ast-grep --lang go --pattern 'type MsgSubmitProof struct { $$$ }')
# Step 2: Extract file paths without line numbers or additional text
msg_submit_proof_files=$(echo "$msg_submit_proof_definition" | grep -oP '^\S+' | sed 's/:.*//g' | uniq)
# Step 3: Check if MsgSubmitProof implements the required methods
for file in $msg_submit_proof_files; do
echo "Checking file: $file"
ast-grep --lang go --pattern 'func (m MsgSubmitProof) GetProof() []byte' "$file"
ast-grep --lang go --pattern 'func (m MsgSubmitProof) GetSessionHeader() *sessiontypes.SessionHeader' "$file"
ast-grep --lang go --pattern 'func (m MsgSubmitProof) GetSupplierAddress() string' "$file"
done
Length of output: 4404
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty clean. Just a couple minor comments!
@@ -43,7 +59,7 @@ type SupplierClient interface { | |||
// session's mined relays. | |||
CreateClaims( | |||
ctx context.Context, | |||
sessionClaims []*relayer.SessionClaim, | |||
claimMsgs ...MsgCreateClaim, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
SessionHeader: sessionProof.SessionHeader, | ||
Proof: sessionProof.ProofBz, | ||
} | ||
msgs := make([]cosmostypes.Msg, 0, len(proofMsgs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we make a copy of the slice? #PUC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While client.MsgSubmitProof
type is compatible with cosmostypes.Msg
, their respective slices are not. We need to create a []cosmostypes.Msg
in order to pass it to SignAndBroadcast
} | ||
|
||
// TODO(@bryanchriswhite): reconcile splitting of supplier & proof modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand what we need to reconcile here. @bryanchriswhite ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client which exposes #CreateClaim()
and #SubmitProof()
should be called ProofClient
; however, the SupplierClient
was written before the Proof
module was split from the Supplier
module. See: #633.
// Type casting does not need to be checked here since the concrete type is | ||
// guaranteed to implement the interface which is just an identity for the | ||
// concrete type. | ||
proofMsg, _ := p.(*prooftypes.MsgSubmitProof) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so is client.MsgSubmitProof
actually client.MsgSubmitProofI
or client.IMsgSubmitProof
.
Maybe this means we should add an I
ass a pre/post fix to make it clearer?
@bryanchriswhite wdyt too? We'd need to do this everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR, I'm not a fan of the I
pre/postfix but would be willing to concede on this detail as it's part of a temporary solution. I think it would be sufficient to move these interfaces to an expected_interfaces.go
and add them on an as-needed basis. A more comprehensive refactoring will be necessary to fully resolve this.
I think that, ideally, the pkg
package SHOULD NOT depend on anything from x/...
. The current issue is that the generated go protobuf types are used both off- and on-chain. So long as go protobuf types are being generated into x/...
, pkg/client/...
(and possibly other packages) will either have dependency cycles which they will have to work around, or such dependency cycles will be looming as several interface types in pkg/client/interface.go
already reference types from x/<module>/types
.
In my view, the correct solution here should involve moving the generated go protobuf types to a package which is not a branch of either pkg/
or x/
(e.g., proto/types/<module>
). This would:
- Resolve this class dependency cycles; eliminating the need for redundant interface message types (i.e., client <-> proto).
- Eliminate on-chain pkg dependencies from leaking through client pkg. This has implications for potential binary size optimzations for off-chain binaries which depend on our protobuf types (e.g., Shannon SDK consumers).
// sessionEndHeight->sessionId->supplierAddress->SessionTree. | ||
// It is used to keep track of the sessions that are created in the RelayMiner | ||
// by grouping them by their end block height, session id and supplier address. | ||
type sessionsTreesMap = map[int64]map[string]map[string]relayer.SessionTree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I loled too 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
load-testing/tests/relays_stress_helpers_test.go (1)
192-192
: Clarify the log message format.The log message format should be consistent with the rest of the codebase. Consider using
Msgf
for formatted messages.- "waiting for next test session to start: in %d blocks", + "waiting for the next test session to start: in %d blocks",
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- load-testing/tests/relays_stress_helpers_test.go (2 hunks)
- load-testing/tests/relays_stress_test.go (1 hunks)
Additional comments not posted (5)
load-testing/tests/relays_stress_test.go (1)
157-158
: LGTM! The addition oftestEndHeight
field is appropriate.The new field
testEndHeight
is added to store the end height of the test. Ensure that this field is used correctly in the file.load-testing/tests/relays_stress_helpers_test.go (4)
220-223
: Ensure proper cancellation context handling.Verify that
s.cancelCtx()
is correctly set up to handle context cancellation.Verification successful
Proper context cancellation handling is verified.
The initialization and usage of
s.ctx
ands.cancelCtx
are correctly implemented inrelays_stress_test.go
andrelays_stress_helpers_test.go
.
relays_stress_test.go
:s.ctx, s.cancelCtx = context.WithCancel(context.Background())
relays_stress_helpers_test.go
: Multiple usages ofs.ctx
and calls tos.cancelCtx()
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the setup of `s.cancelCtx()` for context cancellation. # Test: Search for `cancelCtx` definition and usage. Expect: Proper setup and usage. rg --type go 's.cancelCtx'Length of output: 383
Script:
#!/bin/bash # Description: Verify the setup and usage of `s.ctx` and `s.cancelCtx` in the relevant test files. # Check initialization of `s.cancelCtx` in `relays_stress_test.go` rg --type go 'context.WithCancel' load-testing/tests/relays_stress_test.go # Check usage of `s.ctx` and `s.cancelCtx` in `relays_stress_helpers_test.go` rg --type go 's.ctx' load-testing/tests/relays_stress_helpers_test.go rg --type go 's.cancelCtx' load-testing/tests/relays_stress_helpers_test.goLength of output: 867
220-223
: Ensure proper handling of relay request termination.Verify that the logic to stop sending relay requests and wait for the settlement of claims and proofs is correctly implemented.
Verification successful
Ensure proper handling of relay request termination.
The logic to stop sending relay requests and wait for the settlement of claims and proofs is correctly implemented in the
mapSessionInfoForLoadTestDurationFn
function. The related functionswaitForTxsToBeCommitted
andwaitUntilLatestBlockHeightEquals
also correctly handle waiting for transactions to be committed and ensuring the latest block height matches the target height.
mapSessionInfoForLoadTestDurationFn
: Implements logic to stop sending relay requests when the test duration is reached and wait for settlement.waitForTxsToBeCommitted
: Handles waiting for transactions to be committed.waitUntilLatestBlockHeightEquals
: Ensures the latest block height matches the target height.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of relay request termination logic. # Test: Search for the logic related to stopping relay requests and waiting for settlement. Expect: Correct implementation. rg --type go 'stop sending relays'Length of output: 167
Script:
#!/bin/bash # Description: Search for the implementation details related to stopping relay requests and waiting for settlement. # Locate the function definitions in the file to identify where the logic might be implemented. rg --type go 'func ' load-testing/tests/relays_stress_helpers_test.go # Search for the keyword 'stop' to find the relevant logic for stopping relay requests. rg --type go 'stop' load-testing/tests/relays_stress_helpers_test.go # Search for the keyword 'wait' to find the relevant logic for waiting for settlement. rg --type go 'wait' load-testing/tests/relays_stress_helpers_test.goLength of output: 5731
Script:
#!/bin/bash # Description: Verify the implementation details of mapSessionInfoForLoadTestDurationFn and related functions. # Extract the implementation of mapSessionInfoForLoadTestDurationFn. ast-grep --lang go --pattern $'func (s *relaysSuite) mapSessionInfoForLoadTestDurationFn($_) $_' # Extract the implementation of waitForTxsToBeCommitted. ast-grep --lang go --pattern $'func (s *relaysSuite) waitForTxsToBeCommitted() $_' # Extract the implementation of waitUntilLatestBlockHeightEquals. ast-grep --lang go --pattern $'func (s *relaysSuite) waitUntilLatestBlockHeightEquals(targetHeight int64) $_'Length of output: 14597
209-210
: EnsuretestEndHeight
calculation is correct.The calculation of
testEndHeight
depends ontotalDurationBlocks
. Verify thattotalDurationBlocks
returns the correct value.
210-210
: VerifytestEndHeight
calculation.Ensure that
testEndHeight
is correctly calculated based on the start block height and the total duration of the test.
// this helps to avoid iterating over all sessionsTrees to check if they are ready to be closed. | ||
// sessionTrees is a map of blockHeight->sessionId->supplierAddress->sessionTree. | ||
// The block height index is used to know when the sessions contained in the | ||
// entry should be closed, this helps to avoid iterating over all sessionsTrees |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"should be closed"
Is "closed" an accurate description here? I thought this would be the earliest supplier proof commit height.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely not the earliest proof commit height, but rather the session's end height.
pkg/relayer/session/session.go
Outdated
sessionsToClaimObs, sessionsToClaimPublishCh := channel.NewObservable[[]relayer.SessionTree]() | ||
channel.ForEach( | ||
ctx, | ||
rs.blockClient.CommittedBlocksSequence(ctx), | ||
rs.forEachBlockClaimSessionsFn(supplierAddress, sessionsToClaimPublishCh), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be factored out to a method which returns sessionsToClaimObs
.
pkg/relayer/session/session.go
Outdated
@@ -254,7 +280,7 @@ func (rs *relayerSessionsManager) forEachBlockClaimSessionsFn( | |||
// Separate the sessions that are on-time from the ones that are late. | |||
// If the session is past its grace period, it is considered late, | |||
// otherwise it is on time and will be emitted last. | |||
if sessionGracePeriodEndHeight+int64(numBlocksPerSession) < block.Height() { | |||
if claimWindowOpenHeight+int64(numBlocksPerSession) < block.Height() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The comment references the grace period but this is now in terms of claim window open height.
- Why are we adding a session for the comparison? Maybe #PUC.
pkg/relayer/session/session.go
Outdated
// retryFailedSessionAndDeleteExpiredFn returns a function that retries failed sessions | ||
// and deletes them if the expiration height is reached. | ||
// expirationHeightFn could be either GetProofWindowCloseHeight or GetClaimWindowCloseHeight. | ||
func (rs *relayerSessionsManager) deleteExpiredSessionTreesFn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment doesn't match the function name. The function name seems to match the implementation.
I love this change, makes the lean client part way cleaner. |
You might say it's the "clean client" 🥁 😂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- pkg/client/interface.go (5 hunks)
- pkg/relayer/session/claim.go (6 hunks)
- pkg/relayer/session/proof.go (7 hunks)
- pkg/relayer/session/session.go (14 hunks)
- x/proof/keeper/msg_server_submit_proof_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/proof/keeper/msg_server_submit_proof_test.go
Additional comments not posted (22)
pkg/relayer/session/claim.go (6)
26-27
: Addition ofsupplierClient
parameter is appropriate.The addition of the
supplierClient
parameter is necessary for the new functionality. The logic seems correct.
172-173
: Addition ofsupplierClient
parameter is appropriate.The addition of the
supplierClient
parameter is necessary for the new functionality. The logic seems correct.
111-111
: Improved error handling and logging.The changes improve error handling and logging, which are beneficial for debugging and maintaining the code.
223-223
: Improved error handling and logging.The changes improve error handling and logging, which are beneficial for debugging and maintaining the code.
Line range hint
429-458
: Well-implemented function for deleting expired session trees.The function is well-implemented and necessary for cleaning up expired session trees, preventing reprocessing.
Line range hint
460-472
: Well-implemented function for managing sessions ready to be claimed.The function is well-implemented and necessary for managing sessions that are ready to be claimed.
pkg/relayer/session/proof.go (4)
27-28
: Addition ofsupplierClient
parameter is appropriate.The addition of the
supplierClient
parameter is necessary for the new functionality. The logic seems correct.
178-179
: Addition ofsupplierClient
parameter is appropriate.The addition of the
supplierClient
parameter is necessary for the new functionality. The logic seems correct.
99-99
: Improved error handling and logging.The changes improve error handling and logging, which are beneficial for debugging and maintaining the code.
248-248
: Improved error handling and logging.The changes improve error handling and logging, which are beneficial for debugging and maintaining the code.
pkg/client/interface.go (5)
36-44
: Addition ofMsgCreateClaim
interface is appropriate.The addition of this interface is necessary to avoid cyclic dependencies and improve code modularity.
45-53
: Addition ofMsgSubmitProof
interface is appropriate.The addition of this interface is necessary to avoid cyclic dependencies and improve code modularity.
63-64
: Update toCreateClaims
function is appropriate.The function has been updated to accept variadic parameters of the new interface
MsgCreateClaim
. This change is necessary to align with the new interface and improve function flexibility.
72-73
: Update toSubmitProofs
function is appropriate.The function has been updated to accept variadic parameters of the new interface
MsgSubmitProof
. This change is necessary to align with the new interface and improve function flexibility.
331-331
: Update toBlock
function is appropriate.The function has been updated to return a different type
cometrpctypes.ResultBlock
. This change is necessary to align with the new type and improve function compatibility.pkg/relayer/session/session.go (7)
24-28
: Restructuring ofsessionsTreesMap
is appropriate.The restructuring improves session management and organization.
116-121
: Update toStart
function is appropriate.The update is necessary to support multiple suppliers and improve parallel processing.
142-184
: Update toensureSessionTree
function is appropriate.The update is necessary to support multiple suppliers and improve session management.
Line range hint
211-275
: Update toforEachBlockClaimSessionsFn
function is appropriate.The update is necessary to support multiple suppliers and improve session management.
299-337
: Update toremoveFromRelayerSessions
function is appropriate.The update is necessary to support multiple suppliers and improve session management.
429-458
: Well-implemented function for deleting expired session trees.The function is well-implemented and necessary for cleaning up expired session trees, preventing reprocessing.
460-472
: Well-implemented function for managing sessions ready to be claimed.The function is well-implemented and necessary for managing sessions that are ready to be claimed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
} | ||
} | ||
|
||
func (rs *relayerSessionsManager) supplierSessionsToClaim( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: godoc comment
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 662) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- pkg/relayer/session/proof.go (6 hunks)
- pkg/relayer/session/session.go (14 hunks)
- x/proof/keeper/msg_server_submit_proof_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- pkg/relayer/session/proof.go
- x/proof/keeper/msg_server_submit_proof_test.go
Additional comments not posted (7)
pkg/relayer/session/session.go (7)
116-121
: LGTM!The changes correctly start the claim/proof pipeline for each supplier.
142-184
: LGTM!The changes ensure that the session tree is correctly created and assigned.
Line range hint
211-275
:
LGTM!The changes ensure that the sessions are correctly emitted based on their claim window open height.
299-337
: LGTM!The changes ensure that the session tree is correctly removed.
427-457
: LGTM!The changes ensure that the expired sessions are correctly deleted.
460-474
: LGTM!The changes ensure that the sessions are correctly notified when they are ready to be claimed.
426-426
: LGTM!The changes ensure that the relay is correctly added to the session tree.
…ation-overserviced * pokt/main: [TODO] Blockers @okdas (#674) [TODO_BLOCKER] chore: Remove stale TODO_BLOCKERs (#694) [TODO] refactor: query expiring claims w/ index (#671) [TODOs] `grace_period_end_offset_blocks` validation & cleanup (#667) [Testing] add account balance coverage to account settlement (#670) [Application] Auto-undelegation of unstaked gateways (#692) [RelayMiner] Fix LeanClient mutiple suppliers support (#662)
…-root * pokt/main: [TODO] Blockers @okdas (#674) [TODO_BLOCKER] chore: Remove stale TODO_BLOCKERs (#694) [TODO] refactor: query expiring claims w/ index (#671) [TODOs] `grace_period_end_offset_blocks` validation & cleanup (#667) [Testing] add account balance coverage to account settlement (#670) [Application] Auto-undelegation of unstaked gateways (#692) [RelayMiner] Fix LeanClient mutiple suppliers support (#662)
Summary
This PR introduces changes to:
LeanClient
support for multipleSuppliers
.SessionTree
removal fromRelayMiner
's session trees map.SessionTree
s so they are no longer reprocessed.SessionTree
s of distinctSupplier
sType of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start
; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_test
make test_e2e
PR Testing (only if making code changes)
devnet-test-e2e
label to the PR.make trigger_ci
if you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
MsgCreateClaim
andMsgSubmitProof
for improved session claim and proof handling.Improvements
CreateClaims
andSubmitProofs
functions to support variadic parameters for better flexibility.Block
function for more efficient block querying.Bug Fixes