Skip to content
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

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Dec 12, 2023

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

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make go_develop_and_test
  • Run E2E tests locally: make test_e2e
  • Run E2E tests on DevNet: Add the devnet-test-e2e label to the PR. This is VERY expensive, o only do it after all the reviews are complete.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@bryanchriswhite bryanchriswhite added on-chain On-chain business logic code health Cleans up some code labels Dec 12, 2023
@bryanchriswhite bryanchriswhite added this to the Shannon TestNet milestone Dec 12, 2023
@bryanchriswhite bryanchriswhite self-assigned this Dec 12, 2023
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices branch from 067a310 to fc28802 Compare December 12, 2023 17:09
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/supplier-errors branch from a789bb3 to 7c961b5 Compare December 12, 2023 17:09
@bryanchriswhite bryanchriswhite linked an issue Dec 13, 2023 that may be closed by this pull request
8 tasks
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/supplier-errors branch 2 times, most recently from 9f31147 to 190b58b Compare December 13, 2023 13:29
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices branch from fc28802 to be55be8 Compare December 13, 2023 13:36
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/supplier-errors branch 2 times, most recently from 6ebd664 to 6817364 Compare December 13, 2023 22:06
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices branch from be55be8 to 7579dcf Compare December 13, 2023 22:10
@bryanchriswhite bryanchriswhite changed the title refactor: proof store indices [Supplier] refactor: proof store indices Dec 14, 2023
@bryanchriswhite bryanchriswhite added the supplier Changes related to the Supplier actor label Dec 14, 2023
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices branch from 7579dcf to 80275dd Compare December 19, 2023 18:32
@bryanchriswhite bryanchriswhite changed the base branch from issues/141/refactor/supplier-errors to issues/141/refactor/in-memory-network December 19, 2023 18:38
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/in-memory-network branch 3 times, most recently from 45d83a6 to d6ae216 Compare December 21, 2023 18:40
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices branch from 80275dd to 9419a30 Compare December 21, 2023 18:57
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/in-memory-network branch 2 times, most recently from efab202 to 12f1041 Compare December 21, 2023 21:09
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices branch from 9419a30 to fabfbef Compare December 21, 2023 21:17
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/in-memory-network branch 2 times, most recently from 8bad7dd to 2b1158a Compare January 2, 2024 12:58
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices branch 2 times, most recently from c210a47 to 942bcff Compare January 2, 2024 13:47
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices branch from 942bcff to bfad61c Compare January 2, 2024 15:15
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices branch from bfad61c to 1ab8ed2 Compare January 3, 2024 10:55
@bryanchriswhite bryanchriswhite marked this pull request as ready for review January 3, 2024 11:27
x/supplier/client/cli/query_proof.go Outdated Show resolved Hide resolved
x/supplier/client/cli/query_proof.go Outdated Show resolved Hide resolved
x/supplier/client/cli/query_proof.go Outdated Show resolved Hide resolved
x/supplier/client/cli/tx_submit_proof.go Outdated Show resolved Hide resolved
x/supplier/keeper/proof.go Show resolved Hide resolved
x/supplier/keeper/proof.go Show resolved Hide resolved
x/supplier/keeper/session.go Outdated Show resolved Hide resolved
x/supplier/keeper/session.go Outdated Show resolved Hide resolved
x/supplier/keeper/session.go Outdated Show resolved Hide resolved
…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
bryanchriswhite and others added 2 commits January 9, 2024 16:21
…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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Debug("got sessionId for proof")
Debug("retrieve on-chain session for proof")


logger.
With(
"session_id", sessionRes.GetSession().GetSessionId(),
"session_id", onChainSession.GetSessionId(),
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
}

@bryanchriswhite
Copy link
Contributor Author

Implementation superseded in #327, omitted tests tracked by #141. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Cleans up some code on-chain On-chain business logic supplier Changes related to the Supplier actor
Projects
Status: ✅ Done
2 participants