declare rust #78
declare rust #78
Conversation
|
The problem with all checks failing: The blockifier cannot be seen as it exists in the upper directory, because yet one more library uses it so it has to be shared. The solution could be to somehow "ignore" this dependency (not sure how) or we could move blockifier into this fork, but I think this is a bad idea honestly - I think all subrepos should be kept in the "parent" repo directly (in this case blockifier should be kept in protostar not in cairo (another subrepo for protostar)). I think we can ignore the CI for now. Once the blockifier is more stable (it is either fork or PR is accepted in the original repo), we can just add a |
| @@ -766,17 +803,168 @@ fn execute_core_hint_base( | |||
| } | |||
| } | |||
|
|
|||
| // ========================================= | |||
| // TODO this code comes from cairo/crates/cairo-lang-protostar/ but that package uses | |||
There was a problem hiding this comment.
Should we crate an task for resolving this cyclic dependencies?
There was a problem hiding this comment.
I think the code was in different crate at first but I moved it to cairo-lang-protostar to make it easier to sync with upstream.
There was a problem hiding this comment.
It should be resolved when we integrate with rust CLI and with my PR to compiler removing our modifications to the cairo-land-runner logic (we should not have any modifications in this crate)
There was a problem hiding this comment.
Ok, so what is the action point here?
So after this and Maks' PR, we're good?
Maks, can you link "your PR"?
There was a problem hiding this comment.
Maks, can you link "your PR"?
There is a task for it software-mansion/protostar#1979
There is no PR yet. You will be able to inject your own implementation of HintProcessor into SierraCasmRunner.
| CompilerConfig { ..CompilerConfig::default() }, | ||
| None, | ||
| ) | ||
| .expect("build failed"); |
There was a problem hiding this comment.
This could use some nicer error
What do you mean?
And code below too
Sorry, but it works better when you comment a certain line with a specific comment, now I'm not sure what you meant.
There was a problem hiding this comment.
I meant that errors should use proper sentences so they are useful for end users. Like Building contract to sierra failed instead of just build failed.
| ) -> Result<(), HintError> { | ||
| match hint { | ||
| &ProtostarHint::StartRoll { .. } => todo!(), | ||
| &ProtostarHint::StartWarp { .. } => todo!(), | ||
| &ProtostarHint::StopRoll { .. } => todo!(), | ||
| &ProtostarHint::StopWarp { .. } => todo!(), | ||
| &ProtostarHint::Declare { .. } => todo!(), | ||
| ProtostarHint::Declare { contract, result, err_code } => { |
There was a problem hiding this comment.
Just letting you know that in spare time I am working on modifications to the runner so we can inject it there instead modifing the source code
There was a problem hiding this comment.
Ok, in such a case, I understand that we'll have to just move the code and that's it.
| @@ -766,17 +803,168 @@ fn execute_core_hint_base( | |||
| } | |||
| } | |||
|
|
|||
| // ========================================= | |||
| // TODO this code comes from cairo/crates/cairo-lang-protostar/ but that package uses | |||
There was a problem hiding this comment.
It should be resolved when we integrate with rust CLI and with my PR to compiler removing our modifications to the cairo-land-runner logic (we should not have any modifications in this crate)
karol-bisztyga
requested review from
piotmag769,
cptartur and
MaksymilianDemitraszek
MaksymilianDemitraszek
force-pushed
the
main
branch
from
1fe6d1d to
3c5cff7
Compare
No description provided.