Skip to content

Conversation

@b-garbacz
Copy link
Contributor

No description provided.

@b-garbacz b-garbacz requested a review from Copilot November 15, 2025 00:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces containerization support for the MSB RPC node and implements an IdentityProvider abstraction to decouple wallet dependency from network operations, enabling RPC nodes to run without wallet initialization.

Key changes:

  • Adds Docker and Docker Compose configurations for containerized RPC node deployment
  • Introduces IdentityProvider pattern to support both wallet-based and key-pair-based identities
  • Adds comprehensive documentation for Docker usage and local development setup

Reviewed Changes

Copilot reviewed 11 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/core/network/identity/IdentityProvider.js New identity abstraction supporting wallet and network key-pair strategies
tests/unit/network/IdentityProvider.test.js Unit tests for the new IdentityProvider class
src/core/network/Network.js Integration of IdentityProvider to replace direct wallet usage
src/core/network/services/ValidatorObserverService.js Replaces wallet-enabled check with validator-observer-specific configuration flag
tests/acceptance/v1/rpc.test.mjs Separates writer and RPC nodes in acceptance tests
msb.mjs Disables wallet for RPC mode
package.json Adds new npm scripts for environment-based execution and Docker support
dockerfile Containerization configuration with Pear runtime
docker-compose.yml Docker Compose service definition for msb-rpc
README.md Updated documentation covering installation, usage, and Docker deployment
.dockerignore Docker build exclusions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"prod-rpc": "pear run . ${npm_config_store} --rpc --host ${npm_config_host} --port ${npm_config_port}",
"prod": "NODE_OPTIONS='--max-old-space-size=4096' pear run . ${npm_config_store}",
"prod-rpc": "NODE_OPTIONS='--max-old-space-size=4096' pear run . ${npm_config_store} --rpc --host ${npm_config_host} --port ${npm_config_port}",
"env-prod": "if [ -f .env ]; then set -a; . ./.env; set +a; fi; NODE_OPTIONS='--max-old-space-size=4096' pear run. ${MSB_STORE:-node-store}",
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

Corrected spacing in 'pear run.' to 'pear run .'

Suggested change
"env-prod": "if [ -f .env ]; then set -a; . ./.env; set +a; fi; NODE_OPTIONS='--max-old-space-size=4096' pear run. ${MSB_STORE:-node-store}",
"env-prod": "if [ -f .env ]; then set -a; . ./.env; set +a; fi; NODE_OPTIONS='--max-old-space-size=4096' pear run . ${MSB_STORE:-node-store}",

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +29
#ENTRYPOINT ["npm", "run"]
#CMD ["env-prod-rpc"]
CMD ["tail", "-f", "/dev/null"]
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The CMD is set to 'tail -f /dev/null' instead of executing the application. Either uncomment the ENTRYPOINT/CMD directives to run 'env-prod-rpc' or remove the commented lines if the no-op tail command is intentional for debugging.

Suggested change
#ENTRYPOINT ["npm", "run"]
#CMD ["env-prod-rpc"]
CMD ["tail", "-f", "/dev/null"]
ENTRYPOINT ["npm", "run"]
CMD ["env-prod-rpc"]

Copilot uses AI. Check for mistakes.
- **Dependency install failures** – confirm you are on Node.js v24.11.0 (LTS) and npm ≥ 11.6.1. If packages still fail to build, clear artifacts (`rm -rf node_modules package-lock.json && npm install`) and rerun `npm run test:unit:all`.
- **Unit tests fail only in one runtime** – run the targeted commands (`npm run test:unit:node` or `npm run test:unit:bare`) to isolate regressions, then inspect `tests/unit/unit.test.js` for the failing cases.
- **RPC port already in use** – set `MSB_PORT` to a free value (for example `MSB_PORT=5050 npm run prod-rpc --port=5050`) or free the port with `lsof -i :<port>` as needed.
- **Docker container exits immediately** – check `docker compose logs -f msb-rpc` for missing volume permissions or environment variables; the service requires the mounted `./stores` directory to be writable by the container user.
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The line ends abruptly at position 150 without proper sentence completion. Consider verifying the intended content.

Copilot uses AI. Check for mistakes.
}
}

#resolveIdentityProvider(keyPair, wallet) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add this whole logic (creational) to the thing you called provider. Just pass the whole options to it and it will get whatever is needed. The decision logic would be there.

import { TRAC_NETWORK_MSB_MAINNET_PREFIX } from 'trac-wallet/constants.js';
import b4a from 'b4a';

class IdentityProvider {
Copy link
Contributor

@leonardotc leonardotc Nov 17, 2025

Choose a reason for hiding this comment

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

The problem with this is that this aint exactly a provider, it just delegates. In general, you want something to load and return a strategy to the consumer class. A provider is a... kind of... creational pattern (it is an in-between factory method and abstract factory, i guess). This is like a proxy which falls into: This is https://sourcemaking.com/refactoring/smells/middle-man strategies work cause they share the same interface and therefore dont need a proxy.


constructor(keyPair, networkPrefix = TRAC_NETWORK_MSB_MAINNET_PREFIX) {

if (!keyPair?.publicKey || !keyPair?.secretKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use this data to "bake" a wallet instance. Even if the user deisabled the wallet, doesnt mean they disabled the wallet instance, they disabled their mnemonic. The implementation doesnt need to match the user domain of thinking in a 1:1 basis. What they call wallet is their mnemonic, what we (as developers) call wallet is a set of characteristics from an object that can sign and verify.

@leonardotc
Copy link
Contributor

So yeah, to sum it up: I would use a wallet (and understand the wallet as an interface that can sign). I would use a provider that would receive all the options and create a wallet based on those (in this case, the provider is a bit more then a factory method).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants