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

Improvements required by standard and PSBT dependencies #195

Merged
merged 36 commits into from
Dec 25, 2023
Merged

Conversation

dr-orlovsky
Copy link
Member

A bunch of improvements which appeared from my work on v0.11 in the depending libraries (not touching the validation code)

@dr-orlovsky dr-orlovsky added refactoring Code refactoring *compatibility* Issues affecting compatibility and interoperability labels Dec 15, 2023
@dr-orlovsky dr-orlovsky mentioned this pull request Dec 15, 2023
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 491 lines in your changes are missing coverage. Please review.

Comparison is base (f446eba) 17.3% compared to head (3c3b5bc) 16.6%.

Files Patch % Lines
src/validation/validator.rs 0.0% 207 Missing ⚠️
src/contract/anchor.rs 0.8% 129 Missing ⚠️
src/contract/seal.rs 0.0% 50 Missing ⚠️
src/vm/op_contract.rs 0.0% 44 Missing ⚠️
src/validation/consignment.rs 0.0% 14 Missing ⚠️
src/vm/isa.rs 0.0% 12 Missing ⚠️
src/validation/logic.rs 0.0% 10 Missing ⚠️
src/contract/assignments.rs 0.0% 7 Missing ⚠️
src/contract/bundle.rs 53.8% 6 Missing ⚠️
src/contract/contract.rs 0.0% 5 Missing ⚠️
... and 3 more
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #195     +/-   ##
========================================
- Coverage    17.3%   16.6%   -0.7%     
========================================
  Files          31      32      +1     
  Lines        3491    3677    +186     
========================================
+ Hits          604     609      +5     
- Misses       2887    3068    +181     
Flag Coverage Δ
rust 16.6% <1.6%> (-0.7%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dr-orlovsky dr-orlovsky force-pushed the v0.11 branch 3 times, most recently from 4b1860f to 19d4611 Compare December 17, 2023 09:44
Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

Just looking for some clarification on a few items

@@ -104,14 +120,28 @@ impl TransitionBundle {
}

impl TransitionBundle {
pub fn validate(&self) -> bool {
pub fn validate(&self) -> Result<ContractId, BundleError> {
let mut contract_id = None;
let mut used_inputs = bset! {};
for item in self.values() {
Copy link
Member

Choose a reason for hiding this comment

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

I just want to call attention to the contents of this loop and confirm it's written as it should be... It seems strangely implemented.

impl SealDefinition<GenesisSeal> {
pub fn transmutate(self) -> SealDefinition<GraphSeal> {
impl Xchain<GenesisSeal> {
pub fn transmutate(self) -> Xchain<GraphSeal> {
Copy link
Member

Choose a reason for hiding this comment

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

What does transmutate do? Is it supposed to swap across chains? If so, this might be wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not across chains. Across forms of seals (genesis/extension vs transition).

In transitions the seal may point to a witness tx, which txid is not known. Thus we use special TxPtr enum covering this case. In genesis/extension there is no witness tx and txid of the seal is always known, that is why we use Txid type. The first type of seals is called GraphSeal, the other GenesisSeal

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, good to know

@@ -52,6 +52,22 @@ impl InstructionSet for RgbIsa {
bset! {"RGB"}
}

fn src_regs(&self) -> HashSet<Reg> {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some docs to this file? It's not obvious to me what the does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. It returns set of registers whose values are used as inputs. It is very helpful both for printing execution logs (showing inputs and outputs for each opcode) - but also needed for the future static analyzers and code optimizers (if two instructions share no inputs/outputs they are commutative, meaning they can go in any order/parallelized in execution).

It comes from the trait in AluVM where it has doc comment, so it should be seen here in docs.rs

Copy link
Member

Choose a reason for hiding this comment

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

Cool, I'll look for it there when it's published

Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

ACK

@dr-orlovsky dr-orlovsky merged commit 6c0a818 into master Dec 25, 2023
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
*compatibility* Issues affecting compatibility and interoperability refactoring Code refactoring
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants