Skip to content

Audit and refactoring of commitments workflows. Vesper support#204

Merged
dr-orlovsky merged 32 commits into
masterfrom
vesper
Feb 26, 2024
Merged

Audit and refactoring of commitments workflows. Vesper support#204
dr-orlovsky merged 32 commits into
masterfrom
vesper

Conversation

@dr-orlovsky

Copy link
Copy Markdown
Member

No description provided.

@codecov

codecov Bot commented Feb 3, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 9.44056% with 259 lines in your changes are missing coverage. Please review.

Project coverage is 16.0%. Comparing base (52e9dcb) to head (1759eab).
Report is 2 commits behind head on master.

❗ Current head 1759eab differs from pull request most recent head d4f96c9. Consider uploading reports for the commit d4f96c9 to get more accurate results

Files Patch % Lines
src/contract/id.rs 12.0% 117 Missing ⚠️
src/bin/rgbcore-stl.rs 0.0% 50 Missing ⚠️
src/lib.rs 2.6% 37 Missing ⚠️
src/contract/operations.rs 16.7% 10 Missing ⚠️
src/contract/state.rs 0.0% 10 Missing ⚠️
src/validation/state.rs 0.0% 9 Missing ⚠️
src/contract/bundle.rs 27.3% 8 Missing ⚠️
src/contract/data.rs 0.0% 7 Missing ⚠️
src/contract/attachment.rs 0.0% 4 Missing ⚠️
src/schema/schema.rs 0.0% 3 Missing ⚠️
... and 2 more
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #204     +/-   ##
========================================
- Coverage    16.8%   16.0%   -0.7%     
========================================
  Files          32      33      +1     
  Lines        3896    4001    +105     
========================================
- Hits          653     642     -11     
- Misses       3243    3359    +116     
Flag Coverage Δ
rust 16.0% <9.4%> (-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 marked this pull request as ready for review February 4, 2024 12:32
@oacdutra

oacdutra commented Feb 9, 2024

Copy link
Copy Markdown
Member

Hi @dr-orlovsky,

I noticed that this change causes an error in the contracts of RGB20 and RGB25: Every time we execute the contract_id method, the method returns a different id.

Because this doesn't happen with RGB21, I assume it's related to AssetTags.

!!! Update !!!

After some verification, I noticed this change causes the error: https://github.com/RGB-WG/rgb-core/pull/206/files#diff-bc03f4e1f24cc17c9393d8df61c0d41c5801bd6c8b8b8b58f80a37fbf738b028R467-R468

@dr-orlovsky

Copy link
Copy Markdown
Member Author

@crisdut yeah, I see the reason. Will work on fix

@dr-orlovsky dr-orlovsky force-pushed the vesper branch 2 times, most recently from a07dd51 to 7f40c94 Compare February 11, 2024 11:10

@oacdutra oacdutra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The error remains. Pls see my comments

@oacdutra oacdutra self-requested a review February 14, 2024 01:49

@oacdutra oacdutra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dr-orlovsky,

Pls see my comments.

Comment thread src/contract/operations.rs
Comment thread src/contract/operations.rs
Comment thread src/contract/operations.rs
@dr-orlovsky

dr-orlovsky commented Feb 15, 2024

Copy link
Copy Markdown
Member Author

@crisdut thank you for finding the source of the issue. In 86fb382 I fixed the code such that it is impossible to call a method returning an invalid commitment value.

@oacdutra

Copy link
Copy Markdown
Member

@dr-orlovsky

I tried transfer with all latest version of the crates and I find a new error:

commit encoder can commit only to named types

After investigate, we try commit a unnamed type here (the input_map):

pub struct TransitionBundle {
    pub input_map: Confined<BTreeMap<Vin, OpId>, 1, U16>,
    pub known_transitions: Confined<BTreeMap<OpId, Transition>, 1, U16>,
}

impl CommitEncode for TransitionBundle {
    type CommitmentId = BundleId;

    fn commit_encode(&self, e: &mut CommitEngine) { e.commit_to(&self.input_map); } <------ error here
}

The error occurs because Confined::<BTreeMap<Vin, OpId>, 1, U16>::strict_name() is implemented by:

// ./strict_enconding/rust/src/embedded.rs:547

impl<K: StrictType + Ord + Hash, V: StrictType, const MIN_LEN: usize, const MAX_LEN: usize>
    StrictType for Confined<BTreeMap<K, V>, MIN_LEN, MAX_LEN>
{
    const STRICT_LIB_NAME: &'static str = LIB_EMBEDDED;
    fn strict_name() -> Option<TypeName> { None }
}

While the other types evoke another impl:

// ./strict_enconding/rust/src/types.rs:48
pub trait StrictType: Sized {
    const STRICT_LIB_NAME: &'static str;
    fn strict_name() -> Option<TypeName> {
        fn get_ident(path: &str) -> &str {
            path.rsplit_once("::")
                .map(|(_, n)| n.trim())
                .unwrap_or(path)
        }

        let name = any::type_name::<Self>().replace('&', "");
        let mut ident = vec![];
        for mut arg in name.split([',', '<', '>', '(', ')']) {
            arg = arg.trim();
            if arg.is_empty() {
                continue;
            }
            ident.push(get_ident(arg));
        }
        Some(tn!(ident.join("")))
    }
}

@dr-orlovsky

Copy link
Copy Markdown
Member Author

@crisdut thanks for reporting the details of the source of the error. Fixed in 1759eab

oacdutra
oacdutra previously approved these changes Feb 15, 2024
@dr-orlovsky dr-orlovsky added this to the v0.11.0 milestone Feb 26, 2024
Disclose info and hash ids
@dr-orlovsky dr-orlovsky merged commit d7e42b2 into master Feb 26, 2024
@dr-orlovsky dr-orlovsky self-assigned this Feb 27, 2024
@dr-orlovsky dr-orlovsky deleted the vesper branch April 18, 2024 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants