Refactor: Refactored p2p2core adapater#3225
Conversation
ndatta-nethermind
temporarily deployed
to
Development
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3225 +/- ##
==========================================
+ Coverage 76.64% 76.66% +0.01%
==========================================
Files 323 323
Lines 31863 31863
==========================================
+ Hits 24422 24427 +5
- Misses 5675 5684 +9
+ Partials 1766 1752 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
adapters/p2p2core/class.go
Outdated
| } | ||
|
|
||
| func createCompiledClass(cairo1 *class.Cairo1Class) (*core.CasmClass, error) { | ||
| // todo should this be a method of class.Cairo1Class |
There was a problem hiding this comment.
Is this todo a leftover?
There was a problem hiding this comment.
This was a requirement:
Add a todo on top which asks: "should this be a method of class.Cairo1Class"
| assert.NotNil(t, adaptedResponse.Compiled) | ||
| } else { | ||
| assert.Nil(t, adaptedResponse.Compiled) | ||
| assert.Empty(t, adaptedResponse.Compiled) |
There was a problem hiding this comment.
Why do we change the behaviour?
There was a problem hiding this comment.
Why do we change the behaviour?
There was a problem hiding this comment.
Here is the screenshot of the issue:
None of the above is saying that we should set Compiled to &CasmClass{} instead of nil if the declared class is Cairo V0.
There was a problem hiding this comment.
It's asking to return value type instead of reference type in CompileToCasm method into adapters/p2p2core/class.go.
There was a problem hiding this comment.
- If Cairo V0: Set
Compiledtonil - If Cairo V1: Set
Compiledto&casmClass
There was a problem hiding this comment.
@infrmtcs but if we are adapting Sierra classes, Cairo 0 wouldn't be adapted to it. Where do we adapt Cairo zero and set the Casm to nil?
| assert.NotNil(t, actualClass.Compiled) | ||
| } else { | ||
| assert.Nil(t, actualClass.Compiled) | ||
| assert.Empty(t, actualClass.Compiled) |
There was a problem hiding this comment.
Why do we change the behaviour?
There was a problem hiding this comment.
Why do we change the behaviour?
| result, err := sn2core.AdaptCompiledClass(nil) | ||
| require.NoError(t, err) | ||
| assert.Nil(t, result) | ||
| assert.Empty(t, result) |
There was a problem hiding this comment.
Why do we change the behaviour?
There was a problem hiding this comment.
Why do we change the behaviour?
adapters/p2p2core/class.go
Outdated
| if cairo1 == nil { | ||
| return nil, nil | ||
| //nolint:exhaustruct // intentionally returning an empty CasmClass | ||
| return core.CasmClass{}, nil |
There was a problem hiding this comment.
I'm wondering if this is correct because we instantiate a core.CasmClass in the Cairo 0 case, where *class.Cairo1Class is nil.
There was a problem hiding this comment.
It seems that now we're only calling this function when compiledClass is non-nil, so we can throw an error here instead.
| if compiledClass == nil { | ||
| return nil, nil | ||
| //nolint:exhaustruct // intentionally returning an empty CasmClass | ||
| return core.CasmClass{}, nil |
There was a problem hiding this comment.
I'm wondering if this is correct because we instantiate a core.CasmClass in the Cairo 0 case, where *class.Cairo1Class is nil.
There was a problem hiding this comment.
It seems that now we're only calling this function when compiledClass is non-nil, so we can throw an error here instead.
| ProgramHash: class.Class.ProgramHash, | ||
| SemanticVersion: class.Class.SemanticVersion, | ||
| Compiled: casmClass, | ||
| Compiled: &casmClass, |
There was a problem hiding this comment.
This means we have a zero core.CasmClass instead of nil in deprecated case.
There was a problem hiding this comment.
Yeah, looks like we should have nil instead
| return core.CasmClass{}, nil | ||
| } | ||
|
|
||
| adapt := func(ep *class.SierraEntryPoint) starknet.SierraEntryPoint { |
There was a problem hiding this comment.
Cant we reuse adaptSierra or adaptSierraEntryPoints here?
There was a problem hiding this comment.
No. adaptSierra return core.SierraEntrypoint.
| ProgramHash: class.Class.ProgramHash, | ||
| SemanticVersion: class.Class.SemanticVersion, | ||
| Compiled: casmClass, | ||
| Compiled: &casmClass, |
There was a problem hiding this comment.
Yeah, looks like we should have nil instead
ndatta-nethermind
temporarily deployed
to
Development
GitHub Actions
Inactive
Co-authored-by: Dat Duong <[email protected]> Signed-off-by: ndatta-nethermind <[email protected]>
ndatta-nethermind
temporarily deployed
to
Development
GitHub Actions
Inactive
rodrodros
temporarily deployed
to
Development
GitHub Actions
Inactive
rodrodros
temporarily deployed
to
Development
GitHub Actions
Inactive
| exhaustruct: | ||
| allow-empty-returns: true |
|
This pull request is stale because it has been open 35 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
4 participants
No description provided.