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

WIP: Miniscript final version #764

Open
wants to merge 51 commits into
base: master
Choose a base branch
from

Conversation

joemphilips
Copy link
Contributor

Previous attempt is in #695

Since the spec has changed drastically, I've been re-writing almost everything from scratch 😭

I swear to myself, from next time, I will never start coding until the spec is fixed.

@joemphilips joemphilips mentioned this pull request Nov 8, 2019
<Optimize>true</Optimize>
<DocumentationFile>bin\Release\NBitcoin.TestFramework.XML</DocumentationFile>
</PropertyGroup>
</Project>
Copy link
Contributor

Choose a reason for hiding this comment

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

seems some .csproj have unwanted line-ending changes

select Utils.DictionaryFromList<PubKey, Tuple<HDFingerprint, KeyPath>>(pks.ToList(), fingerPrintAndPath);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

actually some .cs files too, like this

@@ -20,8 +20,8 @@
<EmbedUntrackedSources>true</EmbedUntrackedSources>
</PropertyGroup>
<PropertyGroup>
<TargetFrameworks>net461;net452;netstandard1.3;netstandard1.1;netcoreapp2.1;netstandard2.0</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove net4xx?

result = new PubKey(v);
return true;
}
catch
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks evil, what's the exception type being caught?

Copy link
Contributor

Choose a reason for hiding this comment

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

All of them

Copy link
Contributor

Choose a reason for hiding this comment

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

that's why it's evil; when I'm sure the author only intended to capture one type

@NicolasDorier
Copy link
Collaborator

I swear to myself, from next time, I will never start coding until the spec is fixed.

Spec never fixed until actually somebody use it :p

@NicolasDorier
Copy link
Collaborator

Can you rebase on master?

@joemphilips joemphilips force-pushed the miniscript_final_version branch from a5dd29b to b7534c1 Compare November 18, 2019 04:26
* fix bug in Correctness.TryAndB
* fix debug format
* There are the case which has same serialization format
 for the script, but we have different ast element representation
 (e.g. `and_v(X,v:Y)` and `v:and_v(X,Y)`)
 So the equality criteria for `Terminal<TPk,TPKh>` must be based on
 its script representation.
@@ -40,6 +41,8 @@ public PubKey(string hex)

}

public PubKey() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks wrong.

@@ -0,0 +1,7 @@
namespace NBitcoin.Scripting.Descriptor
{
public class CreateDescriptor
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

@lontivero
Copy link
Contributor

This is an impressive work. What's the plan?

@joemphilips
Copy link
Contributor Author

I suspended this PR until the bitcoin-core decides how they treat Miniscript. (i.e. This PR gets merged or this issue get solved)

It seems that the author is trying to support taproot in Miniscript before it gets merged.
I cannot see when that happen.

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.

4 participants