Skip to content

Commit bfcc732

Browse files
AnkushinDaniilAnkushinDaniil
committed
feat(style): Add code style enforcement and Copilot review instructions
1 parent 6deb6cd commit bfcc732

File tree

10 files changed

+490
-11
lines changed

10 files changed

+490
-11
lines changed

‎.editorconfig‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -83,19 +83,39 @@ csharp_style_expression_bodied_indexers = true:silent
8383
csharp_style_expression_bodied_accessors = true:silent
8484
csharp_style_expression_bodied_lambdas = true:silent
8585
csharp_style_expression_bodied_local_functions = false:silent
86-
csharp_style_var_for_built_in_types = false:silent
87-
csharp_style_var_when_type_is_apparent = false:suggestion
88-
csharp_style_var_elsewhere = false:suggestion
86+
csharp_style_var_for_built_in_types = false:warning
87+
csharp_style_var_when_type_is_apparent = false:warning
88+
csharp_style_var_elsewhere = false:warning
8989

9090
# Coding convention
9191

92-
dotnet_diagnostic.IDE0005.severity = suggestion
92+
dotnet_diagnostic.IDE0005.severity = warning
9393
dotnet_diagnostic.IDE0041.severity = error
9494

9595
# Prefer NO braces around single embedded statements
9696
csharp_prefer_braces = false:warning
97-
# Ensure builds/IDEs surface it (esp. older analysis levels)
9897
dotnet_diagnostic.IDE0011.severity = warning
9998
# Require embedded statements on their own line
10099
csharp_style_allow_embedded_statements_on_same_line_experimental = false:warning
101100
dotnet_diagnostic.IDE2001.severity = warning
101+
102+
#### StyleCop.Analyzers ####
103+
104+
# Disable all StyleCop rules by category, then enable only what we need
105+
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.SpacingRules.severity = none
106+
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.ReadabilityRules.severity = none
107+
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.OrderingRules.severity = none
108+
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.NamingRules.severity = none
109+
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.MaintainabilityRules.severity = none
110+
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.LayoutRules.severity = none
111+
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.DocumentationRules.severity = none
112+
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.SpecialRules.severity = none
113+
114+
# Enable only member ordering rules
115+
dotnet_diagnostic.SA1202.severity = warning # Access modifier order (public > internal > protected > private)
116+
dotnet_diagnostic.SA1203.severity = warning # Constants should appear before fields
117+
dotnet_diagnostic.SA1204.severity = warning # Static members should appear before instance members
118+
119+
# Explicitly disable problematic rules
120+
dotnet_diagnostic.SA0001.severity = none # XML comment analysis disabled
121+
dotnet_diagnostic.SA1135.severity = none # Using directives must be qualified (crashes)

‎.github/copilot-instructions.md‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
# Copilot Code Review Instructions for Nethermind Arbitrum
2+
3+
## Project Context
4+
5+
This is a C# implementation of the Arbitrum execution client as a plugin for Nethermind Ethereum client. Code correctness and style consistency are critical for blockchain consensus.
6+
7+
## Code Style Rules (ENFORCE STRICTLY)
8+
9+
### Type Declarations
10+
- **REJECT** any use of `var` keyword - always require explicit types
11+
- **REJECT** missing access modifiers on class members
12+
13+
```csharp
14+
// CORRECT
15+
Address account = new("0x123...");
16+
List<Transaction> txs = new();
17+
18+
// WRONG - flag these
19+
var account = new Address("0x123...");
20+
var txs = new List<Transaction>();
21+
```
22+
23+
### Member Ordering (CRITICAL)
24+
Flag violations of this order within classes:
25+
1. Constants (`const`)
26+
2. Static fields
27+
3. Instance fields
28+
4. Constructors
29+
5. Properties
30+
6. Methods
31+
32+
### Access Modifier Ordering
33+
Within each member category, order by access level:
34+
1. `public`
35+
2. `internal`
36+
3. `protected`
37+
4. `private`
38+
39+
### Braces
40+
- Single-statement blocks should NOT have braces
41+
- Multi-statement blocks MUST have braces
42+
43+
```csharp
44+
// CORRECT
45+
if (condition)
46+
DoSomething();
47+
48+
// WRONG
49+
if (condition)
50+
{
51+
DoSomething();
52+
}
53+
```
54+
55+
## Performance Requirements
56+
57+
Flag these anti-patterns:
58+
- Unnecessary allocations in loops or hot paths
59+
- Large structs passed by value instead of `in` or `ref`
60+
- Missing `Span<T>` or `Memory<T>` for buffer operations
61+
- LINQ in performance-critical code paths
62+
63+
## Arbitrum-Specific Rules
64+
65+
- ABI encoding must use big-endian byte order (check for `BinaryPrimitives.WriteUInt256BigEndian` usage)
66+
- Gas operations order is critical - flag any gas deduction/refund logic changes
67+
- State transitions must be deterministic - flag any use of `Random`, `DateTime.Now`, `Guid.NewGuid()`
68+
- Precompile changes require careful review - flag any modification to method ID constants or gas costs
69+
70+
## Comments and Documentation
71+
72+
- Comments should explain "why", not "what"
73+
- Prefer XML documentation (`///`) for public APIs
74+
- Remove commented-out code
75+
- No TODO comments without associated issue/ticket reference

‎.github/production-code.instructions.md‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
---
2+
applyTo:
3+
- "src/Nethermind.Arbitrum/**/*.cs"
4+
excludeAgent:
5+
- "coding-agent"
6+
---
7+
8+
# Production Code Review Instructions
9+
10+
## Member Ordering (CRITICAL - ALWAYS CHECK)
11+
12+
Every class MUST have members in this exact order:
13+
14+
```csharp
15+
public class Example
16+
{
17+
// 1. CONSTANTS (const) - public first, then private
18+
public const int MaxValue = 100;
19+
private const string DefaultName = "Unknown";
20+
21+
// 2. STATIC FIELDS - public first, then private
22+
public static readonly ILogger SharedLogger;
23+
private static readonly object s_lock = new();
24+
25+
// 3. INSTANCE FIELDS - public first, then private
26+
public readonly Address ContractAddress;
27+
private readonly IBlockTree _blockTree;
28+
private int _counter;
29+
30+
// 4. CONSTRUCTORS
31+
public Example(IBlockTree blockTree) { }
32+
33+
// 5. PROPERTIES - public first, then private
34+
public string Name { get; }
35+
private int InternalCounter => _counter;
36+
37+
// 6. METHODS - public first, then private
38+
public void Process() { }
39+
private void Helper() { }
40+
}
41+
```
42+
43+
**REJECT** code where:
44+
- Fields appear after constructors
45+
- Properties appear before fields
46+
- Methods appear before properties
47+
- Private members appear before public members in same category
48+
49+
## Type Declarations (CRITICAL)
50+
51+
**REJECT** any use of `var`:
52+
53+
```csharp
54+
// REJECT
55+
var address = new Address("0x123");
56+
var transactions = new List<Transaction>();
57+
var result = GetResult();
58+
59+
// REQUIRE
60+
Address address = new("0x123");
61+
List<Transaction> transactions = new();
62+
MyResultType result = GetResult();
63+
```
64+
65+
## Access Modifiers
66+
67+
**REJECT** missing access modifiers:
68+
69+
```csharp
70+
// REJECT
71+
class MyClass { }
72+
readonly ILogger _logger;
73+
74+
// REQUIRE
75+
public class MyClass { }
76+
private readonly ILogger _logger;
77+
```
78+
79+
## Execution (src/Nethermind.Arbitrum/Execution/**)
80+
81+
Additional checks for execution files:
82+
- Gas deduction order is critical - flag any reordering of gas operations
83+
- Flag changes to refund calculations
84+
- Flag any state modification that could be non-deterministic
85+
86+
## Performance
87+
88+
**Flag** these patterns:
89+
- Allocations inside loops
90+
- LINQ in hot paths (prefer foreach)
91+
- Large structs passed by value
92+
- String concatenation in loops (use StringBuilder)
93+
- Missing `readonly` on fields that could be readonly

‎.github/test-code.instructions.md‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
---
2+
applyTo:
3+
- "src/Nethermind.Arbitrum.Test/**/*.cs"
4+
- "**/Test/**/*.cs"
5+
- "**/*Tests.cs"
6+
- "**/*Test.cs"
7+
---
8+
9+
# Test Code Review Instructions
10+
11+
## Test Naming Convention (CRITICAL - ALWAYS ENFORCE)
12+
13+
**Pattern: `MethodName_Condition_ExpectedResult`**
14+
15+
Every test method MUST have exactly THREE parts separated by underscores:
16+
1. Method/feature being tested
17+
2. State/condition/input
18+
3. Expected behavior/result
19+
20+
### CORRECT Examples (approve these patterns)
21+
```csharp
22+
GetBalance_ValidAccountAndEnoughGas_ReturnsBalance
23+
GetBalance_InsufficientGas_ThrowsOutOfGasException
24+
GetBalance_NonExistentAccount_ReturnsZero
25+
DigestMessage_ValidMessage_ProducesBlock
26+
ParseInput_MalformedData_ThrowsDecodingException
27+
```
28+
29+
### WRONG Examples (REJECT and request fix)
30+
```csharp
31+
// Missing condition part
32+
GetBalance_ReturnsBalance // REJECT: missing condition
33+
TestGetBalance // REJECT: not following pattern
34+
35+
// Informal/verbose language
36+
GetBalance_DoesntHaveEnoughBalance_Fails // REJECT: use "InsufficientBalance"
37+
GetBalance_WhenAccountDoesNotExist_Returns0 // REJECT: use "NonExistentAccount"
38+
39+
// Extra prefixes
40+
Should_GetBalance_WhenValid_ReturnBalance // REJECT: no "Should" prefix
41+
Test_GetBalance_Returns_Value // REJECT: no "Test" prefix
42+
43+
// Vague naming
44+
BalanceTest // REJECT: not descriptive
45+
TestMethod1 // REJECT: meaningless name
46+
```
47+
48+
### Naming Guidelines
49+
50+
**For conditions, use concise terms:**
51+
- `ValidInput`, `InvalidInput`, `MalformedData`
52+
- `EnoughGas`, `InsufficientGas`, `ZeroGas`
53+
- `ExistingAccount`, `NonExistentAccount`
54+
- `EmptyArray`, `NullParameter`, `ZeroValue`
55+
56+
**For results, use action verbs:**
57+
- `ReturnsValue`, `ReturnsZero`, `ReturnsNull`, `ReturnsEmpty`
58+
- `ThrowsException`, `ThrowsArgumentException`
59+
- `ProducesBlock`, `UpdatesState`, `EmitsEvent`
60+
- `Succeeds`, `Fails`
61+
62+
## Test Structure (AAA Pattern)
63+
64+
Require clear Arrange/Act/Assert sections:
65+
66+
```csharp
67+
[Test]
68+
public void GetBalance_ValidAccount_ReturnsBalance()
69+
{
70+
Address account = new("0x123...");
71+
UInt256 expectedBalance = 1000;
72+
73+
UInt256 result = sut.GetBalance(account);
74+
75+
Assert.That(result, Is.EqualTo(expectedBalance));
76+
}
77+
```
78+
79+
## Assertion Rules (CRITICAL)
80+
81+
**REJECT** these assertion patterns:
82+
```csharp
83+
// WRONG - no range comparisons
84+
Assert.That(value, Is.GreaterThan(0));
85+
Assert.That(value, Is.LessThan(100));
86+
Assert.That(value, Is.AtLeast(1));
87+
88+
// CORRECT - exact values only
89+
Assert.That(value, Is.EqualTo(42));
90+
Assert.That(value, Is.EqualTo(expectedValue));
91+
```
92+
93+
**Flag** tests with:
94+
- Multiple unrelated assertions
95+
- Branching logic (if/switch)
96+
- Shared mutable state between tests
97+
- Dependencies on test execution order
98+
99+
## Test Organization
100+
101+
**Flag** these patterns:
102+
- `[SetUp]` or `[TearDown]` methods (prefer self-contained tests)
103+
- Private helper methods (should be in test infrastructure)
104+
- Test classes with dependencies on other test classes
105+
106+
## Type Declarations in Tests
107+
108+
Same as production code - **REJECT** `var` keyword:
109+
110+
```csharp
111+
// CORRECT
112+
IWorldState worldState = TestWorldStateFactory.CreateForTest();
113+
Address account = new("0x123...");
114+
115+
// WRONG
116+
var worldState = TestWorldStateFactory.CreateForTest();
117+
var account = new Address("0x123...");
118+
```

‎.github/workflows/ci.yml‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ jobs:
3131
- name: Setup .NET
3232
uses: actions/setup-dotnet@v4
3333
with:
34-
dotnet-version: '9.0.x'
34+
dotnet-version: '10.0.x'
3535

3636
- name: Build ${{ matrix.project }}
3737
run: dotnet build ${{ matrix.project }} --configuration Release
@@ -51,7 +51,7 @@ jobs:
5151
- name: Setup .NET
5252
uses: actions/setup-dotnet@v4
5353
with:
54-
dotnet-version: '9.0.x'
54+
dotnet-version: '10.0.x'
5555

5656
- name: Install dotnet-format
5757
run: dotnet tool install -g dotnet-format
@@ -72,7 +72,7 @@ jobs:
7272
- name: Setup .NET
7373
uses: actions/setup-dotnet@v4
7474
with:
75-
dotnet-version: '9.0.x'
75+
dotnet-version: '10.0.x'
7676

7777
- name: Run tests with Arbitrum-only coverage (Cobertura)
7878
run: |

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,7 @@ FodyWeavers.xsd
408408
## Nethermind
409409
keystore/
410410
/.githooks
411+
src/Nethermind.Arbitrum.Cleanup.sln
411412

412413
# Test coverage reports
413414
test-coverage/

‎Makefile‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,23 @@ coverage-report: ## Generate and open HTML coverage report in browser
144144
echo "✅ ReportGenerator installed. Run 'make coverage-report' again."; \
145145
fi
146146

147-
format: ## Format code using dotnet format
147+
format: ## Format code using dotnet format (excludes Nethermind submodule)
148148
@echo "Formatting Nethermind Arbitrum code..."
149-
dotnet format src/Nethermind.Arbitrum/Nethermind.Arbitrum.csproj
150-
dotnet format src/Nethermind.Arbitrum.Test/Nethermind.Arbitrum.Test.csproj
149+
dotnet format src/Nethermind.Arbitrum/Nethermind.Arbitrum.csproj --exclude src/Nethermind/
150+
dotnet format src/Nethermind.Arbitrum.Test/Nethermind.Arbitrum.Test.csproj --exclude src/Nethermind/
151+
152+
format-check: ## Verify code formatting (CI validation)
153+
@echo "Checking code formatting..."
154+
dotnet format src/Nethermind.Arbitrum/Nethermind.Arbitrum.csproj --verify-no-changes
155+
dotnet format src/Nethermind.Arbitrum.Test/Nethermind.Arbitrum.Test.csproj --verify-no-changes
156+
157+
reorder: ## Fix member ordering with ReSharper CLI (local use)
158+
@command -v jb >/dev/null 2>&1 || (echo "Installing JetBrains ReSharper CLI..." && dotnet tool install -g JetBrains.ReSharper.GlobalTools)
159+
@cd src && [ -f Nethermind.Arbitrum.Cleanup.sln ] || (dotnet new sln -n Nethermind.Arbitrum.Cleanup -o . --force && \
160+
dotnet sln Nethermind.Arbitrum.Cleanup.sln add Nethermind.Arbitrum/Nethermind.Arbitrum.csproj Nethermind.Arbitrum.Test/Nethermind.Arbitrum.Test.csproj)
161+
cd src && jb cleanupcode --profile="Reorder Members" --settings="Nethermind.Arbitrum.sln.DotSettings" \
162+
--include="Nethermind.Arbitrum/**/*.cs;Nethermind.Arbitrum.Test/**/*.cs" --exclude="Nethermind/**" \
163+
Nethermind.Arbitrum.Cleanup.sln
151164

152165
help: ## Show this help message
153166
@echo "Available targets:"

0 commit comments

Comments
 (0)