stacks-network / stacks-core

The Stacks blockchain implementation
https://docs.stacks.co
GNU General Public License v3.0
3.01k stars 672 forks source link

refactor: add serde default to symbolic expression struct #5479

Closed hugocaillard closed 1 day ago

hugocaillard commented 3 days ago

Context

Clarinet uses clarity with the developer-mode feature enabled.

It also needs to fetch some data from the RPC endpoints from the stacks-nodes (such as contract metadata, including contract body, as SymbolicExpressions). The response from the stacks-node don't include the SymbolicExpression span and other developer-mode properties. As a result, Clarinet can not parse the response of the api call.

Fix

By adding the #[serde(default)] directive, it enables Clarinet to parse the response. It's important to note that n this context, we don't care about the actual values of this properties

hugocaillard commented 3 days ago

@csgui i requested a review, I think that clarity-wasm used to have the developer-mode feature enabled for clarity, but it doesn't seem to be the case anymore

csgui commented 1 day ago

In addition to @obycode's review, could you please check the Unchanged files with check annotations warnings? But only if it doesn’t take too much time. It seems to be related to a test component, but it would be great to ensure all checks are passing.

obycode commented 1 day ago

In addition to @obycode's review, could you please check the Unchanged files with check annotations warnings? But only if it doesn’t take too much time. It seems to be related to a test component, but it would be great to ensure all checks are passing.

I've seen these kinds of things on other PRs. I don't think Hugo caused those.

hugocaillard commented 1 day ago

@obycode Addressed the nit. I also added the skip_serializing_if you suggested, I agree the logic does make sense of skipping ser if it can be defaulted. In practice I don't think it'll have a real impact

csgui commented 1 day ago

@obycode Yes. Warnings were not added by this PR. We can check them in a separate effort.

hugocaillard commented 1 day ago

Thanks @csgui @obycode