Skip to content
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

Fix AMR #214

Merged
merged 4 commits into from
Apr 23, 2024
Merged

Fix AMR #214

merged 4 commits into from
Apr 23, 2024

Conversation

uncomputable
Copy link
Collaborator

Fix an error that was introduced in #212. Fix clippy.

The error was introduced in fac715d

The error makes the AMR computation panic.
uncomputable added a commit to uncomputable/simfony that referenced this pull request Apr 23, 2024
@@ -236,7 +236,7 @@ pub fn decode_expression<I: Iterator<Item = u8>, J: Jet>(
converted.push(new);
}

converted[len - 1].get().map(Arc::clone)
converted[len - 1].get().cloned()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from violating our MSRV, this code is objectively worse because it hides the fact that the clone is merely an arc clone.

If clippy is really complaining about this, you probably need to file separate bugs about (a) the lint not respecting MSRV and (b) the lint being stupid.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was struggling to write .cloned() in a way that includes Arc. The lint is stupid and I disabled it now.

The CMR value of this variant is never used. Maybe the design of the
humanb encoding changed and the Literal variant became useless?
@apoelstra
Copy link
Collaborator

Yeah, I think the design of Literal did change. I believe this was a very early idea for doing disconnect that turned out not to be sensible.

Copy link
Collaborator

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 32d30e4

@apoelstra apoelstra merged commit e3866e8 into BlockstreamResearch:master Apr 23, 2024
16 checks passed
@uncomputable uncomputable deleted the fix-amr branch April 23, 2024 14:44
uncomputable added a commit to BlockstreamResearch/simfony that referenced this pull request Apr 23, 2024
970b841 Cargo: Make WASM compatible (Christian Lewe)
aae8139 Cargo: Remove lazy_static (Christian Lewe)

Pull request description:

  Use the latest rust-simplicity master which is compatible with WASM. The version on crates.io is not yet compatible.

  Depends on BlockstreamResearch/rust-simplicity#214

ACKs for top commit:
  apoelstra:
    ACK 970b841

Tree-SHA512: 95c5df1cddc51d57a12bff7a6cbe69848be768d3f101eba58e88444a2ed562dcd6d61da22e5fca6b8d0957e0863afc6cd4cc10edc8dad6f234403c2780e60281
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants