ElementsProject / elements-miniscript

Creative Commons Zero v1.0 Universal
11 stars 14 forks source link

Add IndexOps Spec #39

Closed sanket1729 closed 1 year ago

sanket1729 commented 1 year ago

Allows more flexible covenant creation. Required for Limit orders implementation.

This will allow many more covenants to be expressed in elements-miniscript. I think this makes us feature complete for the time being :crossed_fingers: . At the very least, I cannot think of us needing any extensions that we are missing

apoelstra commented 1 year ago

e0871e646635701cceddf2f4796595d6cf978be8 looks great -- but do we need to use 64-bit addition for index ops? Seems like we could use the old-school 32-bit addition opcode.

sanket1729 commented 1 year ago

We don't need it, I just used it for consistency with other opcodes.

On Wed, Jan 4, 2023, 11:55 PM Andrew Poelstra @.***> wrote:

e0871e6 https://github.com/ElementsProject/elements-miniscript/commit/e0871e646635701cceddf2f4796595d6cf978be8 looks great -- but do we need to use 64-bit addition for index ops? Seems like we could use the old-school 32-bit addition opcode.

— Reply to this email directly, view it on GitHub https://github.com/ElementsProject/elements-miniscript/pull/39#issuecomment-1371273129, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABUQEONFSWNV4KOMI6BMTI3WQW6C5ANCNFSM6AAAAAATQFTE3Q . You are receiving this because you authored the thread.Message ID: @.***>

apoelstra commented 1 year ago

I think we should drop it, and require that index expressions lie between 0 and 2^31-1. (Or is it 2^32 - 1? I don't recall, but it doesn't matter :)).

sanket1729 commented 1 year ago

Updated with the requested suggestions.

apoelstra commented 1 year ago

Ok, LGTM. I am tempted to say that indices should be bounded below by 0, not 2^31 - 1, because conceptually indices are nonnegative. But I could imagine this being annoying in certain cases where it would impose order-of-operations requirements on arithmetic. And also it would require extra opcodes to enforce this.

Will ACK shortly.

apoelstra commented 1 year ago

We should probably also update the README to indicate the new cargo update -p requirement.

sanket1729 commented 1 year ago

@apoelstra, done in #40