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

Refactor with repository pattern #2540

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

moreal
Copy link
Contributor

@moreal moreal commented Aug 13, 2024

Motivation

This pull request adds the repository pattern. Specifically, it will abstract and incrementally improve the behavior of BlockChain so that BlockChain, TxPool, etc. can be accessed through a repository instead of directly.

This is because we found a problem while working on something else: right now we're using BlockChain directly, so it's hard to test "under what conditions" we want the headless to behave that way. This is because the only way to manipulate the state of BlockChain is to do transactions, create blocks, and expect them to change the state. That's well tested in the Libplanet project, but testing headless implies that as well. This will not only make writing tests difficult, but it will also make the tests take longer to run.

That's why we want to gradually refactor to the repository pattern. I'd appreciate your feedback.

Overview

Repositories

I introduce several new repositories in this pull request. The purpose of the repositories is to separate BlockChain logic (in general HTTP server, Database) and GraphQL representation (in general HTTP server Controller). This makes testing easier because the GraphQL layer only depends on repositories in some parts.

Below are the new repository names and descriptions:

  • IBlockChainRepository: A repository about blocks (BlockChain.Tip, Block)
  • IWorldStateRepository: A repository about states (IWorldState)
  • IStateTrieRepository: A repository about state-trie (MerkleTrie)
  • ITransactionRepository: A repository about transactions, transaction results, nonces.

Testing with mocking

Since this pull request, you don't have to setup any NineChroniclesNodeService and any BlockChain to setup states that you want. You can just mock IWorldStateRepository.

You can manually mock like the below example code:

var worldState = new World(MockUtil.MockModernWorldState)
  .SetLegacyState();
var stateRootHash = worldState.Trie.Hash;
var tip = new Domain.Model.BlockChain.Block(
    BlockHash.FromString("YOUR_BLOCK_HASH"),
    BlockHash.FromString("YOUR_PREVIOUS_BLOCK_HASH"),
    default(Address),  // The block proposer (i.e., miner)
    0,  // The block index.
    Timestamp: DateTimeOffset.UtcNow,
    StateRootHash: stateRootHash,
    Transactions: ImmutableArray<Transaction>.Empty  // Append transactions if you need.
);
BlockChainRepository.Setup(repository => repository.GetTip())
    .Returns(tip);
WorldStateRepository.Setup(repository => repository.GetWorldState(stateRootHash))
    .Returns(worldState);

But if your test don't care any block data and care only states on tip, you can use GraphQLTestBase.SetupStatesOnTip:

// Setup GoldCurrencyState with Currencies.Crystal and mint Currencies.Crystal
SetupStatesOnTip(world => world
    .SetLegacyState(Addresses.GoldCurrency, new GoldCurrencyState(Currencies.Crystal).Serialize())
    .MintAsset(new ActionContext(), userAddress, Currencies.Crystal * 10));

// Setup AvatarState
SetupStatesOnTip(world => world
    .SetAvatarState(avatarAddress, avatarState));

@moreal moreal requested review from ipdae, U-lis and area363 August 13, 2024 00:39
@moreal moreal self-assigned this Aug 13, 2024
@moreal moreal force-pushed the introduce-repository branch from 1128716 to cc53ab7 Compare August 22, 2024 01:50
@moreal moreal force-pushed the introduce-repository branch 9 times, most recently from a5ba925 to 030603c Compare September 6, 2024 03:32
@moreal moreal marked this pull request as ready for review September 6, 2024 03:32
@U-lis U-lis added enhancement New feature or request dx labels Sep 6, 2024
@moreal moreal force-pushed the introduce-repository branch 4 times, most recently from 116012b to 688f8f1 Compare September 11, 2024 10:04
@ipdae
Copy link
Member

ipdae commented Sep 11, 2024

DevEx 테스트가 실패하는데 괜찮은건가요?

@moreal
Copy link
Contributor Author

moreal commented Sep 11, 2024

DevEx 테스트가 실패하는데 괜찮은건가요?

@ipdae 테스트 관련해서 깨지는 것이 아닌 Roslyn 컴파일러 쪽에서 발생하고 있습니다. 최근 계속 발생하고 있는데 원인을 파악하지 못 해 재시도(rerun) 하는 것으로 넘어가고 있습니다

image

@moreal
Copy link
Contributor Author

moreal commented Sep 11, 2024

#2582 이슈로 만들어 놓았습니다

 - Introduce `IBlockChainRepository`.
 - Introduce `IWorldStateRepository`.
 - Introduce `ITransactionRepository`.
 - Introduce `IStateTrieRepository`.
 - Replace the usage of `StandaloneContext.BlockChain` with repositories.
 - Separate `NodeStatusType` GraphQL type and `NodeStatus` record type.
@moreal moreal force-pushed the introduce-repository branch from 688f8f1 to 02a1406 Compare September 20, 2024 05:21
@moreal
Copy link
Contributor Author

moreal commented Sep 20, 2024

Could you review this pull request, when you have time? @ipdae @U-lis

@moreal moreal merged commit 76dccd5 into planetarium:development Sep 20, 2024
9 checks passed
@moreal moreal deleted the introduce-repository branch September 20, 2024 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dx enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants