-
Notifications
You must be signed in to change notification settings - Fork 13
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
[Supplier] refactor: proof store indices #266
[Supplier] refactor: proof store indices #266
Conversation
067a310
to
fc28802
Compare
a789bb3
to
7c961b5
Compare
9f31147
to
190b58b
Compare
fc28802
to
be55be8
Compare
6ebd664
to
6817364
Compare
be55be8
to
7579dcf
Compare
7579dcf
to
80275dd
Compare
45d83a6
to
d6ae216
Compare
80275dd
to
9419a30
Compare
efab202
to
12f1041
Compare
9419a30
to
fabfbef
Compare
8bad7dd
to
2b1158a
Compare
c210a47
to
942bcff
Compare
942bcff
to
bfad61c
Compare
bfad61c
to
1ab8ed2
Compare
…refactor/proof-store-indices * issues/141/refactor/in-memory-network: chore: post-merge refactor chore: review feedback improvements chore: fix comment typo chore: post-merge refactor chore: fix comment chore: add #GetConfig() chore: rename InMemoryCosmosNetwork to InMemoryNetwork chore: update comment chore: review feedback improvements chore: add TODOs [Docs] Load Test #1 - Plan (#286) [Bug] Fix observable error logging (#298) [docs] Relayminer config documentation (#288) [Configs] Add foundation for RelayMiner operation configs. (#284) [SMT] Update to use SMT v0.8.2 (#297) [EventsReplayClient] Fix Replay Client Bugs (#267)
…refactor/proof-store-indices * issues/141/refactor/in-memory-network: fix: sealed config issue in test
…refactor/proof-store-indices * issues/141/refactor/in-memory-network: chore: add BaseInMemoryNetwork#CreateNewOnChainAccount() fixup! chore: rename InitSDKConfig() to InitCosmosSDKConfig() chore: rename InitSDKConfig() to InitCosmosSDKConfig() chore: review feedback improvements refactor: rename & simplify rename: BaseInMemoryNetwork#CreateOnChainAccounts to #FundOnChainAccounts chore: generalize ErrSupplierInvalidAddress and simplify chore: add TODO trigger CI [SMT] Path Bump to v0.8.2 (#305) [Configs] feat: Add stake amount to supplier staking config (#300) [Configs] feat: Add stake amount in the app stake config (#299) trigger CI
Co-authored-by: Daniel Olshansky <[email protected]>
@@ -70,6 +54,9 @@ func (k msgServer) CreateClaim(goCtx context.Context, msg *suppliertypes.MsgCrea | |||
SessionHeader: msg.GetSessionHeader(), | |||
RootHash: msg.RootHash, | |||
} | |||
|
|||
// TODO_TECHDEBT: check if this claim already exists and return an appropriate error |
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.
// TODO_TECHDEBT: check if this claim already exists and return an appropriate error | |
// TODO_BLOCKER: check if this claim already exists and return an appropriate error |
|
||
logger. | ||
With( | ||
"session_id", sessionRes.GetSession().GetSessionId(), | ||
"session_id", onChainSession.GetSessionId(), | ||
"session_end_height", sessionHeader.GetSessionEndBlockHeight(), | ||
"supplier", supplierAddr, | ||
). | ||
Debug("got sessionId for proof") |
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.
Debug("got sessionId for proof") | |
Debug("retrieve on-chain session for proof") |
|
||
logger. | ||
With( | ||
"session_id", sessionRes.GetSession().GetSessionId(), | ||
"session_id", onChainSession.GetSessionId(), |
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.
Since this is a Debug
log, thoughts on adding a bit more data:
`session_id_on_chain`, onchainSession.id,
`session_id_provided`, session.id
) | ||
} | ||
|
||
// NB: it is redundant to assert that the service ID in the request matches the |
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.
❤️ comments like this!
// pair exists for the given service ID or not, respectively. | ||
|
||
// Ensure the given supplier is in the onChainSession supplier list. | ||
var found bool |
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.
OPTIONAL NIT: I have the urge to create a helper for this, but not a strong stance.
if !findSupplier(sessionRes.GetSession().GetSuppliers(), supplierAddr) {
return nil, suppliertypes.ErrSupplierNotFound.Wrapf(
"supplier address %q not found in session ID %q",
supplierAddr,
sessionHeader.GetSessionId(),
)
}
func findSupplier(suppliers []*SupplierType, addr string) bool {
for _, supplier := range suppliers {
if supplier.Address == addr {
return true
}
}
return false
}
Summary
Human Summary
Refactors on-chain proof store to support multiple indices/stores in the multistore (similar to claims): "primary key", "supplier address", and "session end block height".
Issue
SubmitProof
message handling #141Type of change
Select one or more:
Testing
make go_develop_and_test
make test_e2e
devnet-test-e2e
label to the PR. This is VERY expensive, o only do it after all the reviews are complete.Sanity Checklist