Add implementation for AccountReadableKVState #9414
Conversation
Signed-off-by: IvanKavaldzhiev <[email protected]>
Signed-off-by: IvanKavaldzhiev <[email protected]>
Signed-off-by: IvanKavaldzhiev <[email protected]>
Signed-off-by: IvanKavaldzhiev <[email protected]>
Signed-off-by: IvanKavaldzhiev <[email protected]>
Signed-off-by: IvanKavaldzhiev <[email protected]>
Signed-off-by: IvanKavaldzhiev <[email protected]>
Signed-off-by: IvanKavaldzhiev <[email protected]>
| return new Account( | ||
| new AccountID( | ||
| entity.getShard(), | ||
| entity.getRealm(), |
There was a problem hiding this comment.
Are there setters or a builder that can be used here? This is not maintainable.
There was a problem hiding this comment.
Yes, there is a builder. I will adapt it.
There was a problem hiding this comment.
Same comment for all complex pbj types. For simple types with 1-3 like AccountID can probably skip builder.
Signed-off-by: IvanKavaldzhiev <[email protected]>
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.hedera.modularized.state; |
There was a problem hiding this comment.
We shouldn't have a modularized package and everything should be inside mirror web3 package.
| package com.hedera.modularized.state; | |
| package com.hedera.mirror.web3.state; |
| return new Account( | ||
| new AccountID( | ||
| entity.getShard(), | ||
| entity.getRealm(), |
There was a problem hiding this comment.
Same comment for all complex pbj types. For simple types with 1-3 like AccountID can probably skip builder.
| private List<AccountApprovalForAllAllowance> getApproveForAllNfts(Long ownerId, final Optional<Long> timestamp) { | ||
| final var nftAllowances = timestamp.isPresent() | ||
| ? nftAllowanceRepository.findByOwnerAndTimestampAndApprovedForAllIsTrue(ownerId, timestamp.get()) | ||
| : nftAllowanceRepository.findByOwnerAndApprovedForAllIsTrue(ownerId); | ||
|
|
||
| return nftAllowances.stream().map(this::convertNftAllowance).collect(Collectors.toList()); |
There was a problem hiding this comment.
A lot of work was put into AccountDatabaseAccessor to cache nested calls with Suppliers.memoize() to reduce repository calls. Why was this not carried over? This class should basically be a copy of AccountDatabaseAccessor with transformations swapped out with pbj types.
There was a problem hiding this comment.
I also wanted to keep the Suppliers.memoize() strategy. The problem is - in the current approach we use modified copied DTO models, where we added Supplier type for some of the arguments, so that we can pass our Suppliers. Now using the PBJ types we don't have any control - those are generated DTOs based on the protobuf definitions. The whole set or arguments are concrete types and should be eagerly provided.
I'd be happy to hear an alternative approach, keeping the performance benefits, but now using directly the reusable services we must strictly comply with all of the definitions, without having much control.
| import java.util.concurrent.TimeUnit; | ||
| import java.util.stream.Collectors; | ||
| import lombok.extern.java.Log; | ||
| import org.jetbrains.annotations.NotNull; |
There was a problem hiding this comment.
We should not use jetbrains annotations. It looks like services uses the findbugs annotations so not sure why jetbrains is coming. In mirror node, we prefer the standard jakarta annotations jakarta.annotation.Nonnull.
| final var timestampValue = ContractCallContext.get().getTimestamp(); | ||
| final Optional<Long> timestamp = timestampValue != 0 ? Optional.of(timestampValue) : Optional.empty(); |
There was a problem hiding this comment.
It's weird that the two uses of the context timestamp in the codebase is to wrap it into an optional. Hence we should wrap this into an optional once directly in ContractCallContext.setTimestamp().
| final var timestampValue = ContractCallContext.get().getTimestamp(); | |
| final Optional<Long> timestamp = timestampValue != 0 ? Optional.of(timestampValue) : Optional.empty(); | |
| final var timestamp = ContractCallContext.get().getTimestamp(); |
… into 09251-add-account-database-readable-kv-state
…-readable-kv-state
Signed-off-by: IvanKavaldzhiev <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9414 +/- ##
============================================
- Coverage 92.56% 92.56% -0.01%
- Complexity 7047 7084 +37
============================================
Files 914 916 +2
Lines 29810 29929 +119
Branches 3767 3776 +9
============================================
+ Hits 27595 27704 +109
- Misses 1445 1451 +6
- Partials 770 774 +4 ☔ View full report in Codecov by Sentry. |
Signed-off-by: IvanKavaldzhiev <[email protected]>
Signed-off-by: IvanKavaldzhiev <[email protected]>
Signed-off-by: IvanKavaldzhiev <[email protected]>
|
Description:
This PR adds
AccountReadableKVStateimplementation needed for building the read only state layer in mirror node, that will be utilized by the reusable services components.Related issue(s):
Fixes #9251
Notes for reviewer:
Checklist