AST Cleanup: Split After / Older into more specific types?

[JeremyRubin]

Suggestion:

While keeping the parsing and output intact for compatibility, we could modify our internal representations of After and Older to be:

AfterHeight(), AfterTime(), WaitBlocks(), WaitDuration() or an internal enum (e.g., After(Height | Time), Wait(Blocks | Duration)).

In Sapio I accomplish this at the user level, but it would probably make sense for those branches to be distinct. This also allows (as I've done in the library) to narrow the types used in the internal representations from e.g. u32s for relative timelocks to u16s making the values correct by construction. This also rules out using non-standardized values.

Users of the Library from Rust would have the improved interface, which would make it easier to write "correct" generic code as users would be able to be unconcerned with checking inputs u32s for well formedness to a Older/After clause, while users using this as a string interface would see the old API (unless we modify the spec to have the 4 types of clause).

If the more generic API of Older/After is desired, this could be accomplished with a function Older/After (or type with Into) that takes a u32 and downcasts into the appropriate type.

[sanket1729]

Concept ACK. This will clean up some code here too. I can work on this post tapscript

[apoelstra]

concept ack from me too

[apoelstra]

I think we should do this using an internal enum, which we can convert from a u32 and do checks at that time

[JeremyRubin]

if you prefer that route, you'd be welcome to take the code from sapio (i can relicense that segment under MIT license) https://github.com/sapio-lang/sapio/blob/master/sapio-base/src/timelocks.rs, but that might be a bit extra for the goals here (but i've found it pretty ergonomic in my personal use).

[apoelstra]

Thanks for the offer to relicense -- though FYI we are CC0, not MIT. I don't have time today to dive into whether we'd want to go that route or implement our own (smaller) enum.

[JeremyRubin]

CC0 is fine for that as well, if desired.

[tcharding]

With the use of bitcoin v0.29 we now use LockTime types for After/Older - I believe this issue can be closed.

[apoelstra]

I agree -- @JeremyRubin feel free to re-open but I think we have finally done this (and we fixed a pile of bugs in the process :))

[sanket1729]

@apoelstra, I believe this issue want to splits the Terminal::After(PackedLockTime) into Terminal::AfterHeight(Height) and Terminal::AfterBlocks(Blocks). Same thing for sequence. We have not done that yet

[sanket1729]

This will also us to make progress on #435. I will make an attempt on this one this week.

[gerceboss]

@sanket1729 , I would like to contribute in here , can you suggest how can I start to understand the codebase? And can you also give some detail regarding this issue?