-
Notifications
You must be signed in to change notification settings - Fork 4
4 block update sc business hash #42
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
4 block update sc business hash #42
Conversation
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.
Pull request overview
This PR enhances the blockchain service to support immutability proofs for tournament records by implementing a dual-hash verification system. The key additions include a business hash (snap_hash) computed from tournament data for integrity verification, complementing the existing transaction hash (tx_hash) for existence proof. The smart contract now maintains an enumerable mapping of tournament IDs and emits enhanced events with timestamps and computed hashes. Additionally, the codebase undergoes a systematic rename from tx_id to id for the snapshot identifier field.
Key changes:
- Smart contract enhanced with business hash computation, tournament ID tracking array, and existence mapping
- Backend service implements hash verification by comparing locally-computed hashes with blockchain-emitted hashes
- Database schema updated to store both transaction hash and business hash with block timestamp
- Frontend views updated to display the new hash and timestamp fields
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
srcs/blockchain/src/SmartContract/contracts/GameStorage.sol |
Added tournamentIds array, exists mapping, business hash computation in event, and getNbTournamentStored() function |
srcs/blockchain/src/SmartContract/contracts/GameStorage.t.sol |
Updated tests to verify new event parameters including timestamp and business hash |
srcs/blockchain/src/SmartContract/test/GameStorage.test.ts |
Removed TypeScript integration tests (replaced by Solidity tests) |
srcs/blockchain/src/SmartContract/package.json |
Downgraded chai version and added chai-as-promised, removed @types/chai |
srcs/blockchain/src/abi/GameStorage.json |
Updated ABI to reflect smart contract changes: new event parameters and uint8→uint32 type changes |
srcs/blockchain/src/core/GameStorage.utils.ts |
New file implementing business hash computation and event extraction utilities |
srcs/blockchain/src/core/GameStorage.client.ts |
Code formatting improvements (quotes, spacing) |
srcs/blockchain/src/core/database.ts |
Renamed tx_id→id, date_confirmed→block_timestamp, added snap_hash column and indexes |
srcs/blockchain/src/module/block.schema.ts |
Updated schemas and interfaces to reflect field renames and new hash/timestamp fields |
srcs/blockchain/src/module/block.service.ts |
Added hash verification logic, refactored into separate DB and blockchain functions |
srcs/blockchain/src/module/block.controller.ts |
Simplified controller by delegating to new service functions, improved error handling |
srcs/blockchain/src/module/block.routes.ts |
Updated route parameter from tx_id to id |
srcs/blockchain/src/views/index.ejs |
Updated form fields and table columns to display id, snap_hash, and block_timestamp |
srcs/blockchain/src/views/data.ejs |
Updated to reference id instead of tx_id |
srcs/blockchain/src/test/mockData.ts |
Updated mock data to use id instead of tx_id with improved formatting |
srcs/blockchain/src/test/app.test.ts |
Updated test expectation to match new validation error message |
🎯 What You're Building
You're implementing a dual-hash proof system - this is sophisticated blockchain architecture! The transaction hash proves "this data exists on-chain," while your business hash proves "this specific data hasn't been tampered with." Together, they create an immutable audit trail for your tournament system. This is exactly the kind of thinking that separates junior developers from engineers who understand distributed systems.
💪 The Challenge Ahead
I've found several critical issues that need your attention - but here's the thing: you've already demonstrated you can work with complex concepts like keccak256 hashing, Solidity events, and blockchain integration. The bugs I'm flagging aren't about your capability; they're about the details that matter when building production systems. Every senior developer has spent hours debugging parameter mismatches and schema inconsistencies. This is where you learn the discipline of defensive programming.
🚀 Why This Matters
During your evaluation, you'll need to explain why you chose this dual-hash approach, how the integrity verification works, and what happens if the hashes don't match. The issues I've raised - the field naming confusion, the auto-increment conflicts, the event parameter mismatches - these aren't just bugs to fix. They're opportunities to demonstrate you understand data flow through distributed systems. When your evaluator asks "show me what happens when someone tries to modify tournament data," you'll trace through the hash verification, point to the integrity check, and explain how the blockchain makes tampering detectable.
The Path Forward
Fix the critical bugs first (especially the schema and parameter mismatches), then tackle the documentation. Future you - and your teammates - will thank you for explaining why the hash includes timestamp, why parameter order matters, and why you're storing both hashes. Remember: code that works is good; code that works and teaches the next developer is excellent.
"The blockchain doesn't lie, but your code has to ask it the right questions." — Keep building! 🔥
Files not reviewed (1)
- srcs/blockchain/src/SmartContract/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
srcs/blockchain/src/core/database.ts:115
- The
updateTournamentfunction doesn't return any information about whether the update was successful or how many rows were affected. SQLite'srun()method returns an object withchangesproperty that indicates the number of rows modified.
Consider returning this information so callers can verify the update succeeded. If changes is 0, it means no row with that ID existed, which could indicate a logic error.
export function updateTournament(id: number, tx_hash: string, snap_hash: string, date: number) {
try {
updateTournamentStmt.run(tx_hash, snap_hash, date, id);
} catch (err) {
const error: any = new Error(
`Error storing tx_hash in DB: ${(err as any)?.message || String(err)}`,
);
error.code = 'DB_REGISTER_HASH_ERROR';
throw error;
}
}
| export const blockIdSchema = { | ||
| type: 'object', | ||
| properties: { | ||
| tx_id: { type: 'number' }, |
Copilot
AI
Jan 6, 2026
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.
The blockIdSchema has a mismatch between its properties and required fields. The schema defines a property named tx_id but requires a field named id. This inconsistency will cause validation to fail.
The property should be renamed from tx_id to id to match the required field and align with the rest of the codebase changes where tx_id has been renamed to id.
| tx_id: { type: 'number' }, | |
| id: { type: 'integer' }, |
| return { | ||
| tour_id: Number(args.tour_id), |
Copilot
AI
Jan 6, 2026
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.
The parameter name in extractTournamentStoredEvent is tour_id, but the smart contract event emits this field as the first parameter id (representing the tournament ID), not tour_id. This creates a semantic inconsistency.
Looking at the event definition in GameStorage.sol line 20, the first parameter is named tour_id, but this refers to what the backend calls id. The field mapping in line 50 retrieves args.tour_id, which should correctly map to the event's first parameter.
However, there's a deeper issue: the smart contract's storeTournament function takes id as the tournament identifier (line 22 of GameStorage.sol), but the backend passes tournament.id which actually contains what should be the match/transaction ID, while tournament.tour_id contains the actual tournament ID. This semantic confusion between backend field names and smart contract parameter names needs careful validation to ensure the correct data is being stored on-chain.
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.
c'est résolu ? j'ai l'impression qu'on a bien un nom unique tour_id dans le contrat solidity.
Et pour la 2e remarque, je ne suis pas très au clair sur ce qu'on doit faire du hash de transaction. Il apparait juste apparemment dans le SnapshotRow : tx_hash: string | null; et j'imagine qu'il sera affiché dans le DAPP ?
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.
oui c'est resolu
et le tx_hash est dans la Dapp
srcs/blockchain/src/views/index.ejs
Outdated
| <form id="registerForm"> | ||
| <label for="regtxId">tx_id</label> | ||
| <label for="regtxId">id</label> | ||
| <input id="regtxId" name="tx_id" type="text" /> |
Copilot
AI
Jan 6, 2026
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.
The input field's name attribute is still "tx_id" but the label has been changed to reference "id" and the JavaScript code retrieves it as "regtxId". For consistency with the rename from tx_id to id throughout the codebase, this form field's name attribute should be changed to name="id" to match the backend schema expectations and maintain semantic consistency.
| <input id="regtxId" name="tx_id" type="text" /> | |
| <input id="regtxId" name="id" type="text" /> |
| event TournamentStored(uint32 tour_id, uint32 player1, uint32 player2, uint32 player3, uint32 player4, uint32 ts, bytes32 snapshotHash); | ||
|
|
||
| function storeTournament(uint32 id, uint32 p1, uint32 p2, uint32 p3, uint32 p4) external onlyOwner { | ||
| require(tournaments[id].timestamp == 0, "ID already used"); | ||
| tournaments[id] = Tournament(p1, p2, p3, p4, uint32(block.timestamp)); | ||
| emit TournamentStored(tournamentCount, id, p1, p2, p3, p4); | ||
|
|
||
| tournamentCount++; | ||
| require(!exists[id], "Tournament already stored!"); | ||
| uint32 ts = uint32(block.timestamp); | ||
| exists[id] = true; | ||
| tournaments[id] = Tournament(p1, p2, p3, p4, ts); | ||
| bytes32 snapshotHash = keccak256(abi.encode(id, p1, p2, p3, p4, ts)); | ||
| emit TournamentStored(id, p1, p2, p3, p4, ts, snapshotHash); |
Copilot
AI
Jan 6, 2026
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.
The smart contract event parameters use inconsistent naming compared to the struct and function parameters. The event uses tour_id as the first parameter name (line 20), but the function parameter and struct key both use id. This creates confusion about whether this represents a "tournament ID" or just an "ID".
For clarity and consistency:
- Either rename the event parameter to
idto match the function parameter - Or rename the function parameter to
tour_idto match the semantic meaning
Given the PR description mentions adding a "map of tour_id", using tour_id consistently would better communicate the intent.
| uint256[] public tournamentIds; | ||
| mapping(uint32 => bool) public exists; | ||
|
|
||
| event TournamentStored(uint32 match_id, uint32 player1, uint32 player2, uint32 player3, uint32 player4); | ||
| event TournamentStored(uint32 tour_id, uint32 player1, uint32 player2, uint32 player3, uint32 player4, uint32 ts, bytes32 snapshotHash); | ||
|
|
||
| function storeTournament(uint32 id, uint32 p1, uint32 p2, uint32 p3, uint32 p4) external onlyOwner { | ||
| require(tournaments[id].timestamp == 0, "ID already used"); | ||
| tournaments[id] = Tournament(p1, p2, p3, p4, uint32(block.timestamp)); | ||
| emit TournamentStored(tournamentCount, id, p1, p2, p3, p4); | ||
|
|
||
| tournamentCount++; | ||
| require(!exists[id], "Tournament already stored!"); | ||
| uint32 ts = uint32(block.timestamp); | ||
| exists[id] = true; | ||
| tournaments[id] = Tournament(p1, p2, p3, p4, ts); | ||
| bytes32 snapshotHash = keccak256(abi.encode(id, p1, p2, p3, p4, ts)); | ||
| emit TournamentStored(id, p1, p2, p3, p4, ts, snapshotHash); | ||
| tournamentIds.push(id); |
Copilot
AI
Jan 6, 2026
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.
The tournamentIds array is append-only and will grow indefinitely with each tournament stored. This creates a gas efficiency concern for the smart contract, as the array will consume increasingly more storage over time.
Consider the following implications:
- Gas costs: Pushing to the array costs gas, and the cost increases as the array grows
- Storage costs: Each entry persists forever on the blockchain
- Enumeration efficiency: Reading the entire array becomes expensive as it grows
If the purpose is enumeration and proving exhaustiveness (as mentioned in the PR description), document the expected scale (e.g., max tournaments) or consider implementing a more gas-efficient structure if you expect a large number of tournaments.
| const localHash = computeBusinessHash( | ||
| event.tour_id, | ||
| event.p1, | ||
| event.p2, | ||
| event.p3, | ||
| event.p4, | ||
| event.ts, | ||
| ); | ||
| if (localHash !== event.snapshotHash) { | ||
| throw new Error('Business hash mismatch — integrity violation'); | ||
| } |
Copilot
AI
Jan 6, 2026
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.
The new hash computation and verification logic lacks inline documentation explaining the cryptographic approach and business rules.
Consider adding comments that explain:
- Why both
tx_hash(transaction hash) andsnap_hash(business hash) are needed - as mentioned in the PR description: "Le tx hash prouve l'existence, le hash métier prouve l'intégrité" - What data is included in the business hash and why (tournament ID, players, timestamp)
- Why the local hash is compared with the event's hash (integrity verification)
- What security properties this provides
This documentation would help future developers understand the dual-hash verification pattern and its role in proving immutability.
| import * as db from '../core/database.js'; | ||
| import { errorEventMap, RecordNotFoundError } from '../core/error.js'; | ||
| import { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify'; | ||
| import { BlockTournamentInput, BlockTournamentStored } from './block.schema.js'; |
Copilot
AI
Jan 6, 2026
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.
Unused import BlockTournamentStored.
| import { BlockTournamentInput, BlockTournamentStored } from './block.schema.js'; | |
| import { BlockTournamentInput } from './block.schema.js'; |
| import { | ||
| addTournamentBlockchain, | ||
| addTournamentSnapDB, | ||
| storeTournament, |
Copilot
AI
Jan 6, 2026
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.
Unused import storeTournament.
| storeTournament, |
| import type { AppLogger } from "../core/logger.js"; | ||
| import { getGameStorage } from '../core/GameStorage.client.js'; | ||
| import { BlockTournamentInput, BlockTournamentStored } from './block.schema.js'; | ||
| import { FastifyInstance } from 'fastify'; |
Copilot
AI
Jan 6, 2026
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.
Unused import FastifyInstance.
| import { FastifyInstance } from 'fastify'; |
| export function addTournamentSnapDB(logger: AppLogger, data: BlockTournamentInput) { | ||
| logger.info({ event: 'snapshot_register_attempt', tournament: data }); | ||
| const rowSnapId = db.insertSnapTournament(data); | ||
| logger.info({ event: 'snapshot_register_success', tournament: data }); |
Copilot
AI
Jan 6, 2026
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.
Unused variable rowSnapId.
| logger.info({ event: 'snapshot_register_success', tournament: data }); | |
| logger.info({ event: 'snapshot_register_success', tournament: data, rowSnapId }); |
feat(blockhain): verification hash event = rebuild hash with current tournament data fate(Dapp): add field in Db for check proof and verification of data
BREAKING CHANGE: Database fields and schema package-lock.json srcs/blockchain/.env.test.blockchain srcs/blockchain/nodemon.json srcs/blockchain/package.json srcs/blockchain/src/core/GameStorage.utils.ts srcs/blockchain/src/core/database.ts srcs/blockchain/src/module/block.controller.ts srcs/blockchain/src/module/block.routes.ts srcs/blockchain/src/module/block.schema.ts srcs/blockchain/src/module/block.service.ts srcs/blockchain/src/public/input.css srcs/blockchain/src/public/output.css srcs/blockchain/src/public/style-block.css srcs/blockchain/src/views/data.ejs srcs/blockchain/src/views/header.ejs srcs/blockchain/src/views/index.ejs srcs/blockchain/src/views/snapshot-details.ejs srcs/blockchain/src/views/snapshots.ejs
package-lock.json srcs/blockchain/.env.test.blockchain srcs/blockchain/src/SmartContract/ignition/deployments/chain-31337/deployed_addresses.json srcs/blockchain/src/SmartContract/ignition/deployments/chain-31337/journal.jsonl srcs/blockchain/src/core/database.ts srcs/blockchain/src/module/block.controller.ts srcs/blockchain/src/module/block.routes.ts srcs/blockchain/src/module/block.schema.ts srcs/blockchain/src/public/input.css srcs/blockchain/src/public/output.css srcs/blockchain/src/public/style.css srcs/blockchain/src/test/app.test.ts srcs/blockchain/src/test/mockData.ts srcs/blockchain/src/views/header.ejs srcs/blockchain/src/views/snapshots.ejs
srcs/blockchain/src/core/database.ts
Outdated
| try { | ||
| const match = getSnapTournamentStmt.get(tx_id); | ||
| const match = getSnapTournamentStmt.get(id); | ||
| return (match as BlockTournamentInput) || null; |
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.
c'est du détail mais comme il est aussi utilisé comme type de retour, on peut l'appeler BlockTournament ou BlockTournamentDTO
| .code(406) | ||
| .send({ error: { message: err.message, code: err.code } }) | ||
| this.log.error({ tournament: data.tour_id, err: err?.message || err }); | ||
| return reply.code(406).send({ error: { message: err.message, code: err.code } }); |
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.
si j'ai bien suivi, cette réponse API via http est temporaire, et in fine il y aura un retour (succes et erreur) via redis message broker ?
| reply: FastifyReply, | ||
| ) { | ||
| const { tour_id, player1, player2, player3, player4 } = request.body as BlockTournamentInput; | ||
| const rowId = db.insertSnapTournament(request.body as BlockTournamentInput); |
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.
il y a toujours des appels à la couche repository (ici database.ts) dans le controller. Pour faciliter l'évolution du code (ne modifier que service quand on touche à la logique), on pourrait déléguer à une fonction de service meme si dans l'immédiat elle ne ferait que retransmettre a database.ts
| return { | ||
| tour_id: Number(args.tour_id), |
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.
c'est résolu ? j'ai l'impression qu'on a bien un nom unique tour_id dans le contrat solidity.
Et pour la 2e remarque, je ne suis pas très au clair sur ce qu'on doit faire du hash de transaction. Il apparait juste apparemment dans le SnapshotRow : tx_hash: string | null; et j'imagine qu'il sera affiché dans le DAPP ?
| // Body for POST / | ||
| export const blockSchema = { | ||
| type: 'object', | ||
| properties: { |
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.
c'est probablement ce changement qui fait planter le test du build actuellement. Peut-être qu'on peut continuer à retourner des attributs avec le suffixe id car avec player on s'attendrait à un objet.
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.
difficile à revoir de mon côté mais ça semble solide
| "chai": "^5.1.2", | ||
| "chai-as-promised": "^8.0.2", | ||
| "ethers": "^6.16.0", |
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.
on peut désinstaller chai et mocha, ou bien le service en a t il besoin ?
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.
gitignore ?
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.
ajouter **/build-info/ au gitignore ?
| <section class="mb-6"> | ||
| <h2 class="text-lg font-semibold mb-2">Tournament Data</h2> | ||
| <ul class="font-mono text-sm space-y-1"> | ||
| <li>Player 1 (winner): <%= snapshot.player1_id %></li> |
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.
je ne sais plus si si snapshot renvoie avec _id (mais ce nommage semble + cohérent)
rom98759
left a comment
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.
Tout me semble bon, rien à redire, mais beaucoup de fichiers très gros avec les .json, est-ce qu'ils sont obligatoires ou régénérer à chaque lancement ?
in database.ts and test srcs/blockchain/src/SmartContract/ignition/deployments/chain-31337/journal.jsonl srcs/blockchain/src/SmartContract/package-lock.json srcs/blockchain/src/SmartContract/package.json srcs/blockchain/src/core/database.ts srcs/blockchain/src/module/block.controller.ts srcs/blockchain/src/test/app.test.ts srcs/blockchain/src/views/snapshot-details.ejs
…file make/config.mk srcs/blockchain/src/module/block.controller.ts
Update smart contract
These additions will allow us to create the Dapp in order to validate and prove the immutability of our tournament records.
Updated tests to validate the changes
if you want to test the dapp
go to srcs/blockchain/src/SmartContract
npx hardhat node
in other terminal same directory
npx hardhat ignition deploy ignition/modules/GameStorage.ts
npm run dev:block
or npm run dev //test without smart contract