units: Improve Amount type with guarantees on value being in range

Amount type should be definitely improved with MoneyRange-like functionality, since we guarantee at API level that Amount can't exceed consensus value (having internal value made private).
From here: https://github.com/rust-bitcoin/rust-bitcoin/pull/599#pullrequestreview-649739700

To be clear, we don't actually guarantee at the API level that you can't create out-of-range Amounts. You can do this right now by e.g. adding together two large Amounts.
Also as Sanket says we should be careful to check whether we actually want to make out-of-range amounts be unparseable ... does this match Bitcoin Core?
Also what is the sensible range for SignedAmount? If you subtract two Amounts should you get a SignedAmount?

Seems like Bitcoin Core is perfectly willing to decoderawtransaction transactions with outputs in excess of 21MM.

I'm not sure if this is desirable. It's a consensus-level limit, not a data-type limit. It just doesn't belong in this layer, and would be an overhead and artificial constraint. Someone might want to use Amount to calculate some aggregate over history of blockchain, etc and just need ability to store values above the consensus limit. From the user of this library perspective I don't see how protecting my code from going above the consensus limit protects me from anything meaningful.

I think it would be quite nice to limit the amount range - when you write Bitcoin code that deals with amounts you very, very regularly have to check the range to avoid overflow, and doing it at the data type level with Results for arithmetic operations makes it much harder to accidentally have an overflow which can cause panic or incorrect behavior.

Looks like we just need a dedicated Amount21m type to simplify live of wallet devs - and leave Amount as it is

I'm honestly curious - are there any examples showing where consensus limit of 21M is actually useful? My (possibly naive and wrong) intuition tells me that such a check is pretty always redundant and without a merit? Even in consensus code one would check that sum of outputs is larger than sum of inputs, or that the coinbase is smaller or equal to the value dependent on the block height. In any scenario I can come up with, one would need some more important, fundamental checks (like inputs vs outputs in a tx, or LN funding transaction outputs value vs closing channel outputs value), which make 21M limit completely redundant.
For all practical reasons 21M BTC limit does not actually exist anywhere in the system. It's just a derivative of the coinbase reward schedule, that people enjoy talking about it as marketing point, no?

The reason the check exists in Bitcoin  Core at all is due to the money-printing overflow bug in early versions of Bitcoin. Similar overflow bugs can exist in many, many places, including in the comparison checks you mention. Checking for overflow during arithmetic is something or amount type needs to do, and further restricting the range to 21M is just a little bit safer way of doing that.

On Aug 2, 2021, at 01:55, Dawid Ciężarkiewicz @.***> wrote:

I'm honestly curious - are there any examples showing where consensus limit of 21M is actually useful? My (possibly naive and wrong) intuition tells me that such a check is pretty always redundant and without a merit? Even in consensus code one would check that sum of outputs is larger than sum of inputs, or that the coinbase is smaller or equal to the value dependent on the block height. In any scenario I can come up with, one would need some more important, fundamental checks (like inputs vs outputs in a tx, or LN funding transaction outputs value vs closing channel outputs value), which make 21M limit completely redundant.
For all practical reasons 21M BTC limit does not actually exist anywhere in the system. It's just a derivative of the coinbase reward schedule, that people enjoy talking about it as marketing point, no?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.

Similar overflow bugs can exist in many, many places, including in the comparison checks you mention.

Of course they can, but I can't see how 21M is special here. If a bug like this happens and instead of 100M BTC, one is able to produce 10M BTC (still under 21M), or even 1M, or even +500sats more than the right amount, is this anywhere less of a problem? If a LN implementation allows for creating invalid channel refund tx is this not catastrophic?

Checking for overflow during arithmetic is something or amount type needs to do, and further restricting the range to 21M is just a little bit safer way of doing that.

This "just a little bit safer" seems purely "intuitive" in the bad sense of the world. The overflow protection - yes, that one makes sense, and reliably squash whole class of problems. But either overflow protection works, or not. There's no point in second guessing it.
Adding artificial 21M range check everywhere seems like a Voodoo engineering to me, just needlessly wasting performance and limiting the use of the general purpose type for things like aggregates. And the fact that it might "feel" like it is making anything "safer" is just false sense of security.

Sure, its not magic, but 21M also happens to be the lowest possible value you can pick that wont cause issues. Its not at all "voodoo engineering", its being as conservative as possible, which is totally something any self-respecting Bitcoin software should do. There are a few types of things where this could be useful, most especially where we have a value and then user code converts them to satoshis before adding a fixed number of values together. 21M being as conservative as we can be ensures that its potentially a lot harder to hit that overflow in user code, which is an absolutely massive win on our part.
Remember also that Bitcoin's 21M limit fits exactly in a 64 bit float's mantissa with only one extra bit - meaning you can add two 21M-capped values together as a float and not risk precision loss, but you cannot add two arbitrary integers as such.

One more weak argument: having 21M limit hard-coded in non-Core software adds more disincentive to inflationary hard fork. :)

its being as conservative as possible, which is totally something any self-respecting Bitcoin software should do. There

I'm sorry but that's exactly how cargo/voodoo works. Instead of rational case-specific arguments, its about "what a good Bitcoiner should do", "self-respect"  and in-group image, using vague words like "conservative". None of this is good engineering. I'll leave it at that.

There are a few types of things where this could be useful, most especially where we have a value and then user code converts them to satoshis before adding a fixed number of values together.

That's still very vague with jumping to self-assuring conclusion that "its a massive win". What exactly is the scenario here and how did it actually win anything in the end to end scenario? Was money loss / consensus failure prevented? Why was the user code summing stuff up, and why was detecting going over 21M meaningful at all, that a check against 19M or 22M wouldn't do. I guess I'm asking for at least one well explained and thought out example of a realistic enough application, that was saved by that range check where it would fail without it.
I know that I am a PITA here because I put a burden of proof on other people, and maybe all in all I'm in the wrong here and will have to admit it soon, as there is a good argument and example to be made in favor of this, but so far I haven't seen anything more than "it feels safer so it must be a good idea".
The drawbacks of this range check is:

it limits usability of this general purpose type
it has a runtime performance cost
it provides a false sense of safety"

not awfully lot, but enough IMO to ask for a decent justification.

Remember also that Bitcoin's 21M limit fits exactly in a 64 bit float's mantissa with only one extra bit

Off topic, but I pretty much always roll my eyes when I interact with Bitcoin Core and have to deal with damn floats in an accounting software.

One more weak argument: having 21M limit hard-coded in non-Core software adds more disincentive to inflationary hard fork.

@Kixunil I know that you're kidding, but I can't help to notice that it's not really true in any meaningful sense, and it is mixing ideological considerations with mundane implementation aspects, which is not good for any of them.

but so far I haven't seen anything more than "it feels safer so it must be a good idea".

I'm frankly pretty gobsmacked that this is at all controversial. I'm not arguing 21M is somehow a perfect answer, but just checking for overflows is definitely not as good here. As I said, if a user takes Amounts and as_sat()s them, then adds them together, its easy to hit an overflow if we just allow any 64-bit value. That is almost certainly a bug, likely with security implications, in user code. Sure, it would also be resolved if we limited the value to 2^64 / (Reasonable divisor), but why overthink things when we have a perfectly reasonable limit hard-coded in the definition of Bitcoin?
This isn't theoretical, Bitcoin has had critical vulnerabilities as a result of overflow issues, and if we start with values bounded by 21M, you have to add up a lot of 21M-values before you hit an overflow, instead of having to add two. eg in rust-lightning we add two values together all the time, if those values are bounded by 21M, there's no overflow risk.
Frankly, I consider it a rust oversight that overflows aren't checked by default (given the check is literally free in nearly all practical applications today, unless you're literally limited by program code size, which is pretty rare), but it's what it is.

it limits usability of this general purpose type

Does it materially? More broadly, this isn't a general-purpose type, this is a bitcoin amount.

it has a runtime performance cost

If you can identify this in any benchmark anywhere, I'll shut up and move on, but this ignores lots of practical microarchitectural features in modern CPUs. :facepalm:

it provides a false sense of safety

I'm not sure why you'd say that? I don't know why a user would see a 21M check and say "welp, means amounts can never be out of range" instead of just "well, the amounts wont overflow" which is what this provides.

Same here but too many people use floats in practice :man_shrugging:

it's not really true in any meaningful sense

One either has to rewrite all other software or face unreliability. I think this case was actually experienced in electrs when someone tried to use it with a shitcoin (don't remember the details). Yeah, it was mostly a joke but with some element of truth in it.

I consider it a rust oversight that overflows aren't checked by default

IDK, try!(try!(a + b) + c) would be pretty annoying at that time, even with ? it's not nice. Panics are somewhat interesting but AFAIK you can turn debug_assertions on in release builds.

IDK, try!(try!(a + b) + c) would be pretty annoying at that time, even with ? it's not nice. Panics are somewhat interesting but AFAIK you can turn debug_assertions on in release builds.

Yes, I meant panics, similar to the way they run in debug mode. AFAIU, they're undefined (though in practice wrap), and its super obnoxious that they panic in debug by default but don't panic in release by default (I don't care if you can turn it off or not), at least when there's zero runtime cost (which in today's CPUs is true - an untaken branch based on a flag already set in a register is free, unless you're constrained by the extra byte or two of program code).

AFAIU, they're undefined (though in practice wrap)

It's defined to wrap if debug_assertions is off. I think I see how it can be annoying although I've seen worse. (Drop behavior in Swift changing based on mode comes to mind.)

I'm not arguing 21M is somehow a perfect answer, but just checking for overflows is definitely not as good here.

I'm fine with the fact that overflow might not be enough, but the 21M feels like a bandaid. And the fact that it intuitively feels like a no-brainer is a tell tale sign that the problem was not explored enough and there might be better ideas.

As I said, if a user takes Amounts and as_sat()s them, then adds them together, its easy to hit an overflow if we just allow any 64-bit value.

But it only takes (please correct my math)... 8784 additions to hit the overflow with 21M limit. If the user forgot about the overflow when adding stuff, the chances are high that they are not adding just two numbers, but running it in a loop and the bounds of that loop might as well be above above that limit. So this approach still has holes. And the fact that Amount now does some guarantees about the range might as well backfire by making users too careless.
If as_sat is potential source of critical bugs, why not address that? Why have something that returns unprotected u64 so casually, which invites security issues. Maybe as_sats() should return newtype that forbids math or something? That would be the idiomatic Rust way - we have this neat, convenient and powerful type system, why not use it to guarantee that the problem can't happen no matter what?

try!(try!(a + b) + c) would be pretty annoying at that time,

I agree. But what if any addition on Amount returned some AmountOrOverflow that works more like a NaN value that would be checked exactly at the end of all the math?
So this works:
let sum : Amount = amount.add(other_amount).add(another_amount).check()?;

// you can add as much stuff as you wish, it will still force you to check
let sum : Amount = outputs.iter().fold(Amount(0), |a, b| a + b)).check()?;

I think this is totally doable, ergonomic and works 100% of time, no questions asked! No magic numbers and crossed fingers involved, like we're still savages using stone age programming languages. :D
And for users that really really really need u64, you can add amount.get_sats_u64_i_need_integer_overflow_issues_in_my_life() escape hatch, that is a huge eyesore and remind them that they they are playing with fire.
Or maybe there are other designs that fix the problem reliably, with a good tradeoff between user-proofing and ergonomics.

TBH, I find NaN-like behavior interesting even though it's not exactly ordinary Rust.

this approach still has holes

We're not trying to build a perfect solution that solves all problems, that's up to our users. But pick any arbitrary number and there's a number of additions to hit overflow. Eg lightning has up to 400-some-odd outputs per transaction, so that could reasonably be your divisor and you'd pick a number based on that.
If you feel really strongly against 21M, sure, lets pick NNN million, I don't care?

If as_sat is potential source of critical bugs, why not address that? Why have something that returns unprotected u64 so casually, which invites security issues. Maybe as_sats() should return newtype that forbids math or something? That would be the idiomatic Rust way - we have this neat, convenient and powerful type system, why not use it to guarantee that the problem can't happen no matter what?

Because we live in the real world, and users need numbers from time to time. Yes, we should also make Amount not overflow by default. Dropping unchecked addition and replacing it with checked_add and friends is also a good idea, but...
I'm still somewhat astounded that this is even a debate. Yea, its not perfect, but so what. Why is this a bad idea? You haven't in any way communicated why this is seriously limiting in terms of usability? There isn't a "burden of proof" one way or the other here. Its software, if someone writes a patch, it gets weighed against not having that patch, if its better than not, it can land.

Yea, its not perfect, but so what.

Just my opinion, but I can't help but see it as a crude and helpless workaround, that might have been justified in a lesser programming language incapable of more robust approach. It's neither necessary nor sufficient and is making minor but undesirable sacrifice limiting usefulness of otherwise universal interoperability type in the cornerstone crate of composable Rust Bitcoin ecosystem, making it unsuitable for representing things like e.g. total value routed through a LN node over years, value of transactions using segwit over arbitrary period of time, or a total public debt of El Salvador Interplanetary Empire in year 2166, right before the peak of the next debt supercycle.

The limit of 21M could (should?) be enforced for value in impl consensus::Decodable for TxOut and impl consensus:Encodable for TxOut. In this context it makes a lot of sense. Larger values are meaningfully illegal/invalid, these operations are already failable. Seems like a lot of potential overflow attacks would have to go through these two anyway, so it would largely accomplish the same thing, without any downsides, no?
Any reason why TxOut::outputis not Amount? As we argue about adding some checks to Amount, right now user can just take value: u64 straight out TxOut, without ever going through any Amount, start adding them as raw u64 and have overflow issues, no matter if Amount provides any protection or not.
If you accept the idea that Amount should be an universal type across the ecosystem, then it does seem meaningful to have Amount<T = u64>, allowing users to go to smaller or larger underlying types, without loosing all the type-system machinery built to ensure their butts are always covered. Though maybe that's too much already and it makes things more confusing, no sure.
A combination of:

value: Amount on TxOut
21M check during encoding/decoding
"monadic overflow-checking math" on Amount
encouraging using Amount universally across the ecosystem
discouraging using primitives types for values by only ever exposing them via explicitly named methods

would guarantee that no Rust Bitcoin user will ever screw up with value integer overflow, IMO.

Wow such a long discussion...
My thoughts are:

Having a non-overflowing type is a good safety practice, even if it is not enforced by Bitcoin Core and "just" enforced by consensus.
The name of the type probably should be Sats, not Amount
We may have normal u64 type as a way of computing amounts for things that may overflow 21M - like aggregated data in blockchain. That's why we will need to_u64 on Sats
Yes, Sats should be used all accros Tx, PSBT and similar structures and promoted to the usecase when the amount is related to the amount of satoshis for some procedure (and not agregated data on transactions)

It seems this does not contradict to anyone position, or I am missing something?

I'm not convinced Sats and Amount should be two different types, unless you meant renaming. Casting u64 to Amount is already quite annoying and I was hoping we could get rid of it if Amount was used in TxOut. But maybe more importantly it's totally unclear from the name what is the difference in semantics between Sats and Amount.
If we want to define aggregate amount type it should be clear from the name that it is such. e.g. AggregateAmount or AggregateSats maybe there's something shorter. SatStats is quite short but maybe confusing? Anyway, is there an actual application in the wild that computes statistics? I don't remember seeing any. I'm not happy with adding random "may be useful for someone" stuff while there's a bunch of stuff that's not present and some people actually need it.
So I think we could just add the limit to Amount and a comment that it's unsuitable for statistics with suggestion to ping us if someone is writing application that needs such type.

I'm not convinced Sats and Amount should be two different types, unless you meant renaming.

I meant renaming. For "amount" overflowing 21M we already have u64 and u128 types

I would prefer living in a parallel universe with Amount being called Sats but not sure if such wide breakage is worth it. We could phase it in with #[deprecated = "Use Sats"] type Amount = Sats and keep it that way for reasonably long time.

For "amount" overflowing 21M we already have u64 and u128 types

I believe semantics-enforcing types are great but as I said not worth the trouble if nobody is using it.

Well, u64/128 will be used for aggregated data only which will not need to be printed out/parsed from "X BTC/sat" by default - and we may provide simple u64::rem_btc(self) -> (u32, u32) method for those needing to convert u64 sats aggregate into tuple of full bitcoins and satishis reminder.

Casting u64 to Amount is already quite annoying and I was hoping we could get rid of it if Amount was used in TxOut

The biggest problem with raw u64 in TxOut is that it exposes users to carelessly introduce u64-overflow security vulnerabilities, by just jumping straight to math with u64, where Rust will remove the overflow check in release build by default. The idiomatic Rust way is to guide the user of any API to do the right thing. Overflow protection should be opt-out, not opt-in.

It seems this does not contradict to anyone position, or I am missing something?

I am still meh on the whole thing, but I don't think I have anything more to say, and I feel I exhausted my/your time already. It is not all that important anyway, except for the above point.

I've found an interesting use case for values that are guaranteed to be < MAX_MONEY. Casting to i64 is always safe for them.  This is obviously useful for infallible conversion to SignedAmount but a less obvious use case is storing in postgres. Postgres only supports i64, so keeping the value in range guarantees storing will not fail due to value being out of range.

Another case similar to the above: casting to msat is infallible if there's the MAX_MONEY limit. I find this really nice.

Yet another argument for limit: if something like rustc_layout_scalar_valid_range_end gets stabilized we could add it and get layout optimizations. (eg.g. Option<Amount> would have the same size as Amount) Sure, it'll take years but the limit has to be decided before 1.0.

If this type's primary use is for UTXO's value than I guess range check makes sense, but I would recommend renaming such type to (OutputValue? TxOutValue?) and/or making sure to put the exact meaning in the first line of docstring. I guess what bothered me most is that Amount and other names suggests some general purpose use, while this type is intrinsically linked with the use in TxOut and the user needs to understand it's bitsize and range limit. And I think type-system enforced overflow (over-range) checks would be valuable.

While I agree with the sentiment I'm not sure if "UTXO value" is a clear way of documenting this. E.g. BIP21 should also use this restricted value because it will be used to create UTXO but the relationship may not be so obvious to newbies. I don't have the mental capacity to come up with something significantly better right now, so feel free to suggest, will think about it later.

I'd like to move this forward. It seems we agree that having the check there is OK if it's properly documented. Renaming doesn't seem appealing, so I propose to keep the name but change the first line of doc string (shown on docs from page) to:

Unsigned bitcoin amount up to 21 million bitcoins

for Amount and:

Signed bitcoin amount between -21 million bitcoins and 21 million bitcoins included

for SignedAmount.
With detailed explanation in subsequent paragraphs:

This type guarantees that the value doesn't exceed (-)21 million bitcoins and thus operators always panic on overflow (regardless of debug_assertions & co). However unsafe code still must not rely on this property.
Consider using checked_* or saturating_* methods if panics on overflows are not acceptable.
The type should be used in transaction outputs or other contexts where amounts over the limit indicate an error.
It may not be suitable for aggregate data - in block explorers, accounting... Use primitive types or your own newtype for those cases.

Do you want me to work this into #1162? I don't see 1162 going into this release so I have time to resolve the max_value/MAX thing and add your suggestion above.

Just as a note, a SignedAmount with a cap of 21 million does not make a lot of sense because what is this type used for? Its not a UTXO value. What was SignedAmount introduced for? If we introduce an invariant of 21 million to Amount can we get rid of SignedAmount altogether?

Is it possible that the current Amount type is conflating two things

A type for the value field in TxOut
A general purpose integer type useful for representing bitcoin values (including parsing and printing with denomination etc.)

Perhaps if we had two types:

TxAmount - an amount that can represent a utxo value or total transaction value (hard cap of 21 million, only checked_ ops)
Amount- general purpose integer type useful for any bitcoin application (signed, parsing, pretty printing, all the usual ops, checked, wrapping, etc (just like I just added for u5)

Is it possible that the current Amount type is conflating two things

Exactly-o.

SignedAmount with a cap of 21 million does not make a lot of sense

I think you're right.

Perhaps if we had two types

Rather this:
pub struct Amount<Purpose=TxValue>(u64, PhantomData<Purpose>) where Purpose: Purpose;

// lint to avoid confusion if someone accidentally creates `Amount` with useless purpose
pub trait Purpose: sealed::Purpose {}

pub enum TxValue {}
pub enum General {}

impl sealed::Purpose for TxValue {
    const MAX: u64 = 21_000_000__000_000_00;
}

impl sealed::Purpose for General {
    const MAX: u64 = u64::MAX;
}

mod sealed {
    pub trait Purpose {
        const MAX_VAL: u64;
    }
}

impl<T: Purpose> Amount<T> {
    pub const MAX: Amount = Amount(T::MAX);

    // all checks are based on MAX
}

// Keep most APIs the same, the difference is just max value.
// Amount<TxValue> will have try_from_sat instead of from_sat which will be on Amount<General>
// Also From<TxValue> for General and TryFrom<General> for TxValue

Interesting idea, cheers. I'll have a play with it.

IMO: the job of checking 21M limit logically belong in one place: (de)serialization to/from consensus layer. Seems like this new Amount already went above and beyond to prevent any possibility of missing a wrap-around, so I wonder what is 21M limit even giving us anymore? AFAIR in the past making it harder to get a unchecked wraparound in release mode build was the biggest argument for it.
I also wonder if having SignedAmount is needed at all. Is there a single bit of code in rust-bitcoin that actually need it? Is it common enough in the ecosystem that it demands having it in rust-bitcoin? There is no such a thing as negative amounts in the consensus. To me rust-bitcoin should have the "TXO value type" wrapping the u64 and making sure users don't naively screw up overflow handling, and that's kind of it.

How about we just remove SignedAmount, breaks this problem down a little bit.
https://github.com/rust-bitcoin/rust-bitcoin/pull/1167

@dpc sanity checking various other inputs (e.g. BIP21) and proving the specific variable was checked. In my experience it's better to hit errors sooner rather than later.

@dpc sanity checking various other inputs (e.g. BIP21) and proving the specific variable was checked.

BIP21? The payment URL? What exactly would we be checking? That someone didn't ask in the URL to send more than all BTC in existence ever?
I know it's a sacred number to Bitcoiners, but that 21M is just not a very useful check. The only interesting thing is overflow protection, and 21M was only tangentially related.

 This brings the check from inside the code to the boundary where inputs are taken, so error handling is much nicer. Actually implementing a wallet with BIP21 support means that the exact number given in the URI will be eventually placed in tx output so having the same restriction makes perfect sense.

If it was 19M BTC, or even 100K BTC it wouldn't?

Actually implementing a wallet with BIP21 support means that the exact number given in the URI will be eventually placed in tx output so having the same restriction makes perfect sense.

What actually makes sense is comparing the requested payment value with a total balance of inputs available, which must be done anyway in any sane wallet implementation, rendering this whole endeavor completely futile and redundant.

If it was 19M BTC, or even 100K BTC it wouldn't?

Maybe, but we can't tell for sure.

comparing the requested payment value with a total balance of inputs available

Yeah, but then you either have an unwrap in the code that people need to re-check every time the code changes or funny error handling. Bubbling up types is great.

Yeah, but then you either have an unwrap in the code that people need to re-check every time the code changes or funny error handling. Bubbling up types is great.

The code of any sane wallet will have to check the value from the invoice again available balance and handle the condition. It is going to need to be able to bubble this error up, all the way to the user. That is the only check that was ever needed, infinitely more strict precise. Having a superfluous arbitrary limit does not make anything simpler.
I'm kind of giving up on it. To me it seems that this meaningless 21M number is too hard to let go of, and it's too tempting to be smart about knowing it and thus putting it somewhere. Seems like most people believe it just has to be good to have it.
So we end up contemplating two types and even considering some sealed generic shenanigans, for something that could have been just a single u64 wrapped in overflow checking newtype. Not the end of the world, I guess.

FWIW after reading this thread multiple times and working on various things in Amount, SignedAmount, and other integer types I tend to agree with @dpc. I don't really feel like I have much to add to the conversation but I wanted to register my gut feeling that Amount should just wrap a u64 and have nothing to do with 21 million.

After reviewing https://github.com/rust-bitcoin/rust-bitcoin/pull/1338 I realized that monadic handling is probably really better.
In that PR, overflow checking was initially forgotten and even if it did panic, it'd lead to a DoS vector - someone sending a bad PSBT could cause a service to crash. Monadic handling fixes this. I'd prefer a different design than the PoC crate by @dpc though:
// Maybe this could be just `Result<Amount, OverflowError>`  but I'm not sure if coherence will let us
#[derive(Copy, Clone, Debug, PartialEq, PartialOrd, Hash)] // no Eq, No Ord
pub enum UncheckedAmount {
    Valid(Amount),
    Overflow(OverflowError),
}

impl UncheckedAmount {
    pub fn to_result(self) -> Result<Amount, OverflowError> {
        match self {
            Valid(amount) => Ok(amount),
            Overflow(error) => Err(error),
        }
    }

    // unwrap and expect analogous to those on Result
}

/// Information about overflow
///
/// With `extra-debug-info` feature it carries the location of the code which caused overflow.
/// This will make error paths slower but much easier to debug.
// make sure `Debug` writes location in a usable format - if not write it manually.
#[derive(Copy, Clone, Debug, Hash)]
pub struct OverflowError {
    #[cfg(feature = "extra-debug-info")]
    location: &'static core::panic::Location<'static>,
}

// impl PartialEq, PartialOrd by always returning false/None
// impl Display and std::error::Error; conditionally display panic location (maybe with `#` though?)

The conditional inclusion of overflow location resolves one of my main dislikes about monadic handling. (The other one being non-standard Rust, which I think is outweighed by the added security here.)

Nice @Kixunil, I like this a lot. I agree that trying to reason "by hand" about overflow in all cases is tiring and difficult to get right even when you notice all of the locations of potential overflow.

even when you notice all of the locations of potential overflow

Exactly this. I've made a pretty good habit of checking them but adding another review-modify-publish cycle to already strict and slow process is not great.

In that PoC crate I made the type generic over T, but I do think that Amount should just have it all built-in.

This is one I want, glad we agree on it. ;)
In your PoC they are structs parametrised over Checked. I find enum simpler. I'm happy to expose the internal representation of the enum because there is no reason to change it. Perhaps someone will find it nicer to match on it. Or if it turns out Result works we can get rid of one more type and we can skip to_result().

Am I understanding this enormous thread correctly?

Add a new UncheckedAmount type as posted by @kixunil above.
Add a way to convert Amount easily to UnheckedAmount
Remove all ops from Amount and put them on UncheckedAmount
Expect users to use UncheckedAmount any time they want to do arithmetic with amounts
Keep using Amount in TxOut
Close #620 as not needed.

Is it possible that the current Amount type is conflating two things

Exactly-o.

I was amused to get to the bottom of this thread only to  find I posted this comment 2 years ago and have not changed my view in the slightest.

Seems like Bitcoin Core is perfectly willing to decoderawtransaction transactions with outputs in excess of 21MM.

To be clear -- we do not want to support constructing TxOuts with bad amounts, despite the fact that Core can be coerced into doing it?
Otherwise, rereading Kix's suggestion, I like this. I much prefer the custom enum to trying to use Result<Amount, Overflow> directly as an "unchecked amount" because Result does not support monadic addition.
I also think we could generalize the enum to
enum UncheckedAmount {
    Valid(u64),
    Overflow, // at this point you are just screwed
    Negative(i64), // maybe allow recovering from this and still converting to an amount if it goes positive, maybe by calling a `.ok_if_this_was_temporarily_negative()` method to get back to a `Valid`?
}

Unsure. We should move to a new thread though because this one is a bear.

Before I get lost trying to read blog posts on category theory; what is monadic addition?

To be clear -- we do not want to support constructing TxOuts with bad amounts

I have a feeling I'm about to say something stupid; is the idea that TxOut has vout: UncheckedAmount but the API guarantees it is always UncheckedAmount::Valid (as an invariant and having vout private)?

what is monadic addition

Monadic addition means you can add a Result<number, Err> to a Result<number, Err> and it'll do the right thing (adding the numbers if both are present and the addition is permitted, otherwise producing an error).

I have a feeling I'm about to say something stupid; is the idea that

No, if we want to allow invalid amounts in TxOuts then we should just bail on the UncheckedAmount idea entirely. I was asking an honest question -- is this the direction we want to go with this API?
FWIW no on-chain transaction, even on test chains, ever has outputs that exceed MAX_MONEY. (They do in Elements but Elements needs its own amount type anyway.) (And they do in e.g. dogecoin, but dogecoin also needs special overflow handling and can't naively use Bitcoin amount types, even though they did try initially..)

Perhaps we should have just left TxOut::vout as a u64? And accept that an Amount is a monster of a thing that does parsing, displaying, and any other "random" thing that folk want to do with it.
At least if its a u64 people know they have to handle overflow themselves, and do it in a suitable way for their usecase.
 We could then put checked_* methods on it for arithmetic and call it a day.

Perhaps we should have just left TxOut::vout as a u64?

I find the current API really good (with vout: Amount with type). The ability to construct Amount with several nicely named constructors and I don't have think about number of zeros I am adding in u64 sats etc.
Am I missing something, what is the issue with Amount doing stringy things like parsing?

I won't oppose any change; but I am happy with the current status quo with Amount and SignedAmount.
We can sum over amounts by going via SignedAmount and I don't find value in restricting by MAX_MONEY check.

This thread has some old comments that looking back, I think we can answer a bit more now:

If you can identify this in any benchmark anywhere, I'll shut up and move on, but this ignores lots of practical microarchitectural features in modern CPUs. 🤦

@TheBlueMatt yes, I wrote some benchmarks for coin-selection that showed the checked version of Amount impacts performance a lot, which in the end affects the resolution of the search.  Currently in branch and bound coin selection both core and the rust implementation I benchmarked, we limit the iterations at 100_000 because that's how slow modern CPUs are.  Faster performance means higher possible iterations means better found solutions.

I also wonder if having SignedAmount is needed at all. Is there a single bit of code in rust-bitcoin that actually need it?

@dpc yes, actually, the effective_value of an input during a coin-selection process can be negative.  IE the fee amount subtracted from the TxOut amount may be negative (will not get us anywhere towards a spend target Amount).

I also use SignedAmount for distinguishing debits and credits in wallet code. We definitely want to keep that type.
@tcharding lol, we definitely do not want to go back to value being a u64. Aside from the lack of type-safety, this resulted in sketchy and lossy unit conversions all over user code. And constants with names like OUTPUT_AMOUNT_SATS. As Sanket says, the current situation is way better than that.
The questions here is whether we want to introduce an additional constraint on Amount which is that it can never exceed MAX_MONEY.
@yancyribbens I appreciate that the branches in checked_add etc are slowing down your code. But the proposed UncheckedAmount will not help that at all because it retains all the branches. For your case you need to use u64s and then convert to Amount at the end.

The questions here is whether we want to introduce an additional constraint on Amount which is that it can never exceed MAX_MONEY.

I think that's reasonable.  And for cases where people want to aggregate amounts larger than MAX_MONEY, they can also use a u64 or a different type.

I wonder if we just want to use u64 (or i64) for this rather than adding a new UncheckedAmount type.
The monadic type is nice .. but if users have to know about it, and explicitly construct it, then I don't really see what type-safety benefit there is vs just saying "Amount is range-limited and has no non-checked arithmetic; convert to a scalar type if you want this".

I wonder if we just want to use u64 (or i64) for this rather than adding a new UncheckedAmount type.

The issue is that you lose type safety then and can accidentally mixup different u64 values.  I do think it's a good idea to have Amount as an infallible type and then another fallible type (one without range checking that can panic or overflow).

I was thinking more about this, and maybe u64/i64 is fine since downstream can just wrap it in their own type if desired.

Please see #3691

@apoelstra:

I wonder if we just want to use u64 (or i64) for this rather than adding a new UncheckedAmount type. […] what type-safety benefit there is vs just saying "Amount is range-limited and has no non-checked arithmetic; convert to a scalar type if you want this"

Maybe not type-safety benefits, but there are some advantages in having UncheckedAmount. Amount also deals with units (sats vs. btc et al.), so adding two amounts together is safe regarding units. Also formatting and parsing is useful. These would be beneficial for UncheckedAmount, too.
@yancyribbens:

maybe u64/i64 is fine since downstream can just wrap it in their own type if desired.

I see a lot of potential for bringing dangerous bugs. Sure, computing with i64 is simple enough, but getting correct i64 value in a first place is not. People spent non-trivial amount of time to make parsing of strings and converting units work correctly in this library. It should be reused.

Having two types, one enforcing 21M, one not, sounds good to me. Perhaps the enforcing one does not need to support arithmetic operations within itself, it could delegate them to the non-enforcing one (Amount + Amount = UncheckedAmount, Amount * i64 = UncheckedAmount etc.).
The reason for having two types is that Amount offers some features, like parsing and formatting and dealing with units, that would be useful even for Amount-like type that does not enforce 21M. Bare i64 does not provide these features and would delegate the burden on downstream.

I see a lot of potential for bringing dangerous bugs. Sure, computing with i64 is simple enough, but getting correct i64 value in a first place is not. People spent non-trivial amount of time to make parsing of strings and converting units work correctly in this library. It should be reused.

@jirijakes I think you missed my point.  I had meant to use Amount and SignedAmount except where that type doesn't conform.  And example given above is if Aggregating Amounts of a lighting channel for example, in which case use a different type.  Or in my case where Amount addition is too slow in some cases, cast to u64 in loop and then cast back to Amount before returning from the hot path.
By not limiting to MAX_MONEY we obviate every crate to do a check for example, because to not make this check can lead to potential catastrophic bugs.  By limiting the Amount to type to MAX_MONEY, there's a number of checks that are redundant (and won't be forgotten).
Lastly, another case were this type of safety measure would have prevented this bug I discovered while fuzzing.  Essentially I had an unhanded case where if Amount is greater than i32::MAX you don't get a value at all when calculating an effective_value.

@jirijakes it wouldn't be "one type enforcing nonnegativitiy and MAX_MONEY", one not. It would be "one type enforcig MAX_MONEY' and "one type enforcing u64::MAX (which is only ~8700 times MAX_MONEY) and nonnegativity(?)".
IOW if we define our own type we need to make a bunch of ad-hoc decisions about what its new range should be, how it handles overflows in the negative direction, etc. And the user needs to discover that this type exists and discover all our ad-hoc decisions about it. And then any reader of the code needs to also discover these decisions.
If you actually wanted display and parsing and stuff that would be even more complicated for us, since we have no floating-point type that can hold values larger than MAX_MONEY (times 2.2). So we'd need to substantially rewrite stringification-as-BTC logic, which nobody wants to do.
OTOH, users can use to_sats and from_sats and further converting to i128 or whatever, which has 100% clear semantics to all readers and authors.

@jirijakes my take on your example ledger that has a value of 22_000_000 BTC is that it shouldn't be an Amount type, but rather a ledger type defined by the application at hand.  I think the Amount type should reflect the universal set of total Satoshis in existence.  IE there can't be more than 21 Million, and tying to build a transaction for example with more than that isn't possible with the type.  I haven't thought through your point about intermediate calculations rn, although my instinct is that internally those calculations can still be done by switching in and at of u64 types.

@apoelstra, @yancyribbens: thanks for your explanations, I understand your positions.


Clear parent links

This PR allows us to clear the parent link of all our Taxons via the PublishLinks service class.
By specifically setting a link type to be an empty array, we are able to clear existing links for that link type. Otherwise, PublishLinks works as usual by appending links.
Furthermore, this PR adds a rake task that talks to the publishing API and allows us to clear all existing parent links for all taxons. We only have 100 taxons at the moment, which means that performing this via a rake task should be fine.
Trello task: https://trello.com/c/BAyP2r6c/187-fix-outdated-link-expansion-of-parent-taxons-clean-up-parent
irb(main):001:0> content_id = "e184c8d6-7142-4637-ae1e-3270247e0f4f"
=> "e184c8d6-7142-4637-ae1e-3270247e0f4f"
irb(main):002:0> Services.publishing_api.get_links(content_id)
=> #<GdsApi::Response:0x007fd9a7fbd668 @http_response=<RestClient::Response 200 "{\"content_i...">>
irb(main):003:0> Services.publishing_api.get_links(content_id)['links']
=> {"parent"=>["502eac33-76ad-499e-b15f-a1544be548c5"], "parent_taxons"=>["502eac33-76ad-499e-b15f-a1544be548c5"]}
irb(main):004:0> Taxonomy::ClearParentLink.call(content_id)
=> #<GdsApi::Response:0x007fd9a809a630 @http_response=<RestClient::Response 200 "{\"content_i...">>
irb(main):005:0> Services.publishing_api.get_links(content_id)['links']
=> {"parent_taxons"=>["502eac33-76ad-499e-b15f-a1544be548c5"]}
irb(main):006:0>

@tijmenb Sorry, I should have been clearer. These classes should be deleted after we run the rake task. I added these classes so I could test how this would work with our existing service classes and make sure it would do the right thing. They don't need to stay in the codebase after we run this. But if prefer, I can rework this PR now...


Issue while converting word zero

Eg:- I have zero apples and four oranges. got converted to "I have  apples and f4oranges"

a quick fix could be first replace all .replace(/(zero)/i,0) then do other conventions

Fixed in v1.5.0


Make sure plugin options are passed on to plugins

Description
Options can be provided to plugins by passing an array instead of
the plugin name, with as first element the plugin name, and an
options object as the second argument. However, this array was
destructured as follows:
  originalLocation = originalLocation[0]
  options = originalLocation[1] || {}

Because the first line overwrote originalLocation with the name
of the plugin, the second line would actually assign the second
character of that name to options, rather than the second element
of the array. Switching them around fixes that.
Changes/Tasks

[x] Changed code
[x] Added tests (see notes at the bottom)

Motivation and Context
Plugins could not actually receive options. This PR fixes that.
Types of changes

[ ] Refactoring/add tests (refactoring or adding test which isn't a fix or add a feature)
[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

[ ] My change requires a change to the documentation.

[ ] I have updated the documentation accordingly.

[x] My changes have tests around them

Note that I am not too satisfied with the way I structured the tests, especially the part where I modified the actual code to set a plug-in's name to mock-plugin when in the test environment (as opposed to undefined as used to be the case). I then mocked extra-fs to return true when checking whether the mock-plugin plugin exists, so that I could provide a mock for that plugin.
Unfortunately, I couldn't think of a better way to test this. If anyone knows of a better way I'm all ears, but otherwise I guess we'll have to make do with this.

This is perfectly sufficient for now. You rock!

Thanks Tanner! One more question: at what point do you cut new releases? This and #912 are the only roadblocks to allowing for actual type checking in the TypeScript plugin :)
(No rush! Just trying to get a better picture of my own timelines.)

I'll do a release by the end of the day. Maybe sooner.
On Fri, Dec 28, 2018 at 8:52 AM Vincent<EMAIL_ADDRESS>wrote:

Thanks Tanner! One more question: at what point do you cut new releases?
This and #912 https://github.com/nozzle/react-static/pull/912 are the
only roadblocks to allowing for actual type checking
https://gitlab.com/Vinnl/react-static-plugin-typescript/merge_requests/1
in the TypeScript plugin :)
(No rush! Just trying to get a better picture of my own timelines.)
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/nozzle/react-static/pull/925#issuecomment-450380676,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFUmCYtaPwx6mRwBnYJJmhuzqxbftlugks5u9j4rgaJpZM4Zj39k
.

Ah, so very soon. Again, no rush - it's unlikely that I'll be able to get to it this weekend anyway :)


TRG 4.06 Link to the source of your base image (Container registry and GitHub if available)

There is a new TRG diccussed here Issue #360
That is about to create a separate notice file and include it into the DockerHub
https://eclipse-tractusx.github.io/docs/release/trg-0/trg-4-06

resolved in #201
released in 2.0.3


Skipping link https://pypi.python.org/packages/c0/27/de0e8f452c66eefccb76a029aa8072c6c6f5cc94a52632a6ffed8bcd8f4c/scipy-0.15.0-cp27-cp27mu-manylinux1_x86_64.whl#md5=c92f3760a6735754c6d9bc379cbe4b67 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/c5/26/27b835eaee43982b065ee3fe5fa8ad9c412972bb0becab9fd0ae39932531/scipy-0.14.1rc1.dev_205726a-cp27-none-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl#md5=25c79e26aef2696a651c611b113c4965 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Found link https://pypi.python.org/packages/c7/ce/993b23695a83da2ecd58b083e95ccc162257b24f273299d4d88b6eac5507/scipy-0.14.1.zip#md5=4bfc35d3683e0ef397157f84040df5b0 (from https://pypi.python.org/simple/scipy/), version: 0.14.1
    Skipping link https://pypi.python.org/packages/c8/59/e3745842c71d31c3d2be3aead63b946a09da621aef88fc6182cb210294d1/scipy-0.13.3-cp33-cp33m-macosx_10_6_intel.whl#md5=ca634955b1757fe5e22c6a8a035bb328 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Found link https://pypi.python.org/packages/cb/9d/f6dcd4b43160f4798e52ff75c489a74a6363aa9ee71eaedc6f915238ec0d/scipy-0.16.1-cp27-cp27m-manylinux1_x86_64.whl#md5=eec1c5913a17e1f60988ba5a3c0bbd72 (from https://pypi.python.org/simple/scipy/), version: 0.16.1
    Skipping link https://pypi.python.org/packages/cb/da/b758668af3d1436f5810b04bc54b7d86b061130fa55e1e9c21a839096a64/scipy-0.16.1-cp33-cp33m-manylinux1_x86_64.whl#md5=e4c94aeff45c4f8225b1031506af765d (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/cf/d8/2fcdc72c0e5e893c35f19e6fe784942abaa119b2374abbcb52420e755d3f/scipy-0.10.1-cp27-cp27mu-manylinux1_x86_64.whl#md5=1c0540c0273ba3eb68a8d51b8b122537 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/d2/4b/d58ddeb42d3b9374d86afc216f8841fc15503fa3ee9a56aa9f69f7409251/scipy-0.14.1-cp33-cp33m-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl#md5=423c77aa343f76c30788166400696f68 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Found link https://pypi.python.org/packages/d5/00/5eeed7c67fa5ca80dad12c399bb31450733f0a874a2c8c4d9a8e8ab281c0/scipy-0.13.3.zip#md5=20ff3a867cc5925ef1d654aed2ff7e88 (from https://pypi.python.org/simple/scipy/), version: 0.13.3
    Skipping link https://pypi.python.org/packages/d6/2c/c97267e4f22e48ad110161b5346a7673a38841de76904f343e739a837c93/scipy-0.13.0-cp34-cp34m-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.whl#md5=dcd20dad203d301f0b5ec7aa0bb49aa3 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/da/85/0889544b47bcd30ae1341c0bb6e20720afb890134332cdc34071ab9b6800/scipy-0.14.1-cp34-cp34m-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl#md5=1c9eca63055d0e64b9b9d241ec5f9c0f (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/db/c4/6e2a7f46c9486c6185c60ab0718e7a1b42a971a96b063c6b6e247b7a01fa/scipy-0.15.0-cp34-cp34m-manylinux1_x86_64.whl#md5=a0aef54316cdf75e5fc40f2e7b9a938d (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Found link https://pypi.python.org/packages/dd/f0/222bb29fae425bd482cad6d5f284e63a752ddb28039003d0de310a8773f6/scipy-0.14.1-cp27-cp27m-manylinux1_x86_64.whl#md5=81ad6eba89aac213f4fbf5e204a7edc8 (from https://pypi.python.org/simple/scipy/), version: 0.14.1
    Found link https://pypi.python.org/packages/df/c4/ba1327f80b5663943e0cfa80c640ea3372fd2b65e42cc5159726fe02cf0a/scipy-0.13.0-cp27-cp27m-manylinux1_x86_64.whl#md5=6c3778a88852edca05a164345193120f (from https://pypi.python.org/simple/scipy/), version: 0.13.0
    Found link https://pypi.python.org/packages/e0/6b/82f474751b0606436b8e95b69dbd6a6c15b55420c8a1f7aad6408018c8b0/scipy-0.13.2.tar.gz#md5=fcd110802b0bf3505ba567cf831566e1 (from https://pypi.python.org/simple/scipy/), version: 0.13.2
    Skipping link https://pypi.python.org/packages/e1/9a/c0f5e7f0e73de51fd93682f4a43a79383b2ff3585d7b25170da5d9034ea1/scipy-0.16.0-cp27-cp27mu-manylinux1_x86_64.whl#md5=53725579d80f7118cfb79dab0a2edd90 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/e2/51/5ce532e6edc6770472e72edf9299d8bca7f31576bc45b33e93e250573ab2/scipy-0.14.0-cp26-cp26mu-manylinux1_x86_64.whl#md5=699f733b2850b3793cf4517cceba368a (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/e2/a9/21f537f6561ae56e063f1b704620ff2e1c88fc55f82d9265f9c5e73f7eb9/scipy-0.16.0-cp33-cp33m-manylinux1_x86_64.whl#md5=94b63402dfe4685ccc71436c3e7ab083 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/e2/d8/cafe07f71d6acd838a53aa874242190f6b690ccde4b0bcb94df717e347fc/scipy-0.14.1-cp26-cp26mu-manylinux1_x86_64.whl#md5=18790f7d021dbf05bea0e3eccf144f5d (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Found link https://pypi.python.org/packages/e4/81/df8b2598e99fe1651d70a80760732c95472d8cc3204d0750b8cb47e54525/scipy-0.9.0-cp27-cp27m-manylinux1_x86_64.whl#md5=77d0ff60d18961256f6a754e27cd435a (from https://pypi.python.org/simple/scipy/), version: 0.9.0
    Found link https://pypi.python.org/packages/e5/a5/504bb317d437fad1b5343c6e0068516f2a7adc20cd4977202f1f84a2e7cf/scipy-0.17.0-cp27-cp27m-manylinux1_x86_64.whl#md5=c0c2f7d5dc4c509eb21c0ad04689a46d (from https://pypi.python.org/simple/scipy/), version: 0.17.0
    Skipping link https://pypi.python.org/packages/e5/f5/bbfa7b7c8d9d0bfb8bd45fa7a7f8a62be9f5af74bf9f3fec402bcfa99501/scipy-0.15.1-cp33-cp33m-manylinux1_x86_64.whl#md5=66367d0bb7f5773a5e42e5b83666b8ac (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/e7/c7/9eecef11ac159979d043800face03757ca64e9e1c8985bf6d1db5d424cc6/scipy-0.13.3-cp26-cp26mu-manylinux1_x86_64.whl#md5=644f9b77a711d32739aaefd0d2d04b28 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/eb/2e/76aff3b25dd06cab06622f82a4790ff5002ab686e940847bb2503b4b2122/scipy-0.17.1-cp34-cp34m-manylinux1_x86_64.whl#md5=bb39b9e1d16fa220967ad7edd39a8b28 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Found link https://pypi.python.org/packages/eb/8b/cb7e8eb19e1ee365afcf885ddaccee87e0ec0946ba06b9a3a479dbfaff3c/scipy-0.16.1.zip#md5=f473f9cd366daf4106003accff32c25f (from https://pypi.python.org/simple/scipy/), version: 0.16.1
    Found link https://pypi.python.org/packages/f2/77/0ff9b0f46acb634a781a0e1c389a0258eb1afb7444b36b7a6d09f004f995/scipy-0.12.1.zip#md5=3f23065fc45152c92c3588dad2f20c62 (from https://pypi.python.org/simple/scipy/), version: 0.12.1
    Skipping link https://pypi.python.org/packages/f4/9c/3166dc1181bbb3bbc12ba8a9cea24d2f646f7a0bbe102ac1a5de2675d1ea/scipy-0.13.2-cp34-cp34m-manylinux1_x86_64.whl#md5=b90f0ad41df64b0b4e284c04f8766c07 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/f9/27/6b42b47677b435c3b60bf7b6a39da93aff70c93803b402a03648c5a1fcd3/scipy-0.14.0-cp33-cp33m-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.whl#md5=0ff3c20cb8e9c5c38e5a18cda3178ca7 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/f9/f8/fce4a6bac663bd7ecb9106e9e7ef6492a47f9140d489d7d04091507308b4/scipy-0.16.1-cp26-cp26mu-manylinux1_x86_64.whl#md5=0735aae26686ae02f7e0da11f6188d1b (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Found link https://pypi.python.org/packages/fb/06/571aa713e585b3529dad8838ce2c67178e68c37c5c838772282c83faea68/scipy-0.10.1.tar.gz#md5=6ad976549e22e04ca93e70cf55b70a22 (from https://pypi.python.org/simple/scipy/), version: 0.10.1
    Skipping link https://pypi.python.org/packages/fc/1b/984a997a6e1af1d55a05bafb9b0fcbf008a9114ff0549476c1fd25db44d6/scipy-0.17.0-cp34-cp34m-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl#md5=c6c64c6511ba0de09ca973aaf881b354 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/fc/4e/9d106d322165d4b5c75b859d79c984ab93c12525863245265d9cfee042d7/scipy-0.15.1-cp27-none-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl#md5=a8cf565477600cbedcd5462f2baeeb0e (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/fd/d9/1cb8ab24adeb3ffca0fa5ac8c1875bdb0452a773a68095c7824ced4b4f2e/scipy-0.16.0-cp34-cp34m-manylinux1_x86_64.whl#md5=7dca6350615b0b32619f8b437cffac97 (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
    Skipping link https://pypi.python.org/packages/fe/5a/44dd48306b28f47e08e2daf8c19827ebf99ff8cf983fabe92d9408bc6bd1/scipy-0.14.1rc1.dev_52fb336-cp34-cp34m-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl#md5=45c8e4c39aff1eb30b870796e039adce (from https://pypi.python.org/simple/scipy/); it is not compatible with this Python
  Using version 0.17.1 (newest of versions: 0.8.0, 0.9.0, 0.10.0, 0.10.1, 0.11.0, 0.12.0, 0.12.1, 0.13.0, 0.13.1, 0.13.2, 0.13.3, 0.14.0, 0.14.1, 0.15.0, 0.15.1, 0.16.0, 0.16.1, 0.17.0, 0.17.1)
  Looking up "https://pypi.python.org/packages/05/5e/973bf71cfa865d962a68893e35e366a0a7ac0b713bc398b4e584c1bed982/scipy-0.17.1.tar.gz" in the cache
  No cache entry available
  "GET /packages/05/5e/973bf71cfa865d962a68893e35e366a0a7ac0b713bc398b4e584c1bed982/scipy-0.17.1.tar.gz HTTP/1.1"<PHONE_NUMBER>6
  Downloading scipy-0.17.1.tar.gz (12.4MB)

Okay, it works now, I can only assume that you uploaded scipy-0.17.1-cp27-cp27m-manylinux1_x86_64.whl (md5, pgp) yesterday and that is why it works now, closing again.

I've no idea if somebody uploaded it and who that somebody was. It'd be good to automate the build process for tge upcoming release 0.18 so that it's done better than 0.17.1 (mea culpa)

Ah - no - mea culpa :)   It was me who uploaded it.  I found the problem while trying to improve manylinux testing and deployment - current sketch working for astropy at:
https://travis-ci.org/MacPython/astropy-wheels/builds/137716186
https://github.com/MacPython/astropy-wheels/blob/master/.travis.yml
Just now working on numpy, I hope to get to scipy soon.

Thank you for uploading it @matthew-brett ! And thank you for the whole machinery.


computed mounting options

This seems pretty important. How did we forget?

Apparently this was never documented, and just happened to work from how we pass everything from the mounted options to the component instance using ...options.
No need to support it, I guess.

So this is official removed in Composition API? There's no way to set initial computed value right?


error divide by zero with some background image

Hi,
with some drawable as background image, i have an error divide by zero :
System.err: java.lang.ArithmeticException: divide by zero System.err:     at com.github.sumimakito.awesomeqr.AwesomeQRCode.getDominantColor(AwesomeQRCode.java:377) System.err:     at com.github.sumimakito.awesomeqr.AwesomeQRCode.render(AwesomeQRCode.java:116) System.err:     at com.github.sumimakito.awesomeqr.AwesomeQRCode.create(AwesomeQRCode.java:92) System.err:     at com.github.sumimakito.awesomeqr.AwesomeQRCode.access$700(AwesomeQRCode.java:22) System.err:     at com.github.sumimakito.awesomeqr.AwesomeQRCode$Renderer$1.run(AwesomeQRCode.java:526)

I think the issue may be caused by a background image with no pixels satisfying the statement r<200 && g<200 && b<200, which is used for computing the dominant color without involving too much "light" colors since the foreground and background color of a QR code should not be quite similar.
Anyway, the issue was fixed in commit cef384468a7fc0c075c2ef748252f86f03041e70.
Cheers 🍻


allow jsontoyaml.com to get JSON from a url

way good idea

This is what curl is for:
curl https://raw.githubusercontent.com/coolaj86/json2yaml/master/example.json | json2yaml

At the moment, this can fail if the site is slow to respond, due to https://github.com/coolaj86/json2yaml/pull/11


[Global Procedure] table 7321 "Warehouse Shipment Line" - CalcBaseQty

We have two extension fields on table "Warehouse Shipment Line":
"Requested Qty. to Pick A109"
"Req. Qty. to Pick (Base) A109"
and would like to use code in the Validate trigger of one of them
:
"Req. Qty. to Pick (Base) A109" := CalcBaseQty("Requested Qty. to Pick A109", FieldCaption("Requested Qty. to Pick A109"), FieldCaption("Req. Qty. to Pick (Base) A109"));
but allas procedure CalcBaseQty is set as local. Could you make it global?
And if you want some extra work, search for 'local procedure CalcBaseQty' in the Base source and set them all to global.

Thanks for reporting this. We agree, and we’ll publish a fix asap, either in an update for the current version or in the next major release. Please do not reply to this, as we do not monitor closed issues. If you have follow-up questions or requests, please create a new issue where you reference this one.
Build ID: 10134.


Fix FindJob

Codecov Report

Merging #161 (f9fa4b2) into master (4c70503) will decrease coverage by 1.64%.
The diff coverage is 1.49%.

@@             Coverage Diff              @@
##             master     #161      +/-   ##
============================================
- Coverage     70.60%   68.95%   -1.65%
  Complexity      787      787
============================================
  Files           128      130       +2
  Lines          2745     2812      +67
  Branches        352      360       +8
============================================
+ Hits           1938     1939       +1
- Misses          693      758      +65
- Partials        114      115       +1

Impacted Files
Coverage Δ

...main/java/seedu/address/commons/core/Messages.java
0.00% <ø> (ø)

...ddress/logic/commands/applicant/SortApplicant.java
0.00% <0.00%> (ø)

.../seedu/address/logic/parser/AddressBookParser.java
48.48% <0.00%> (-1.52%)
:arrow_down:

...in/java/seedu/address/logic/parser/ParserUtil.java
69.10% <0.00%> (-4.18%)
:arrow_down:

...ss/logic/parser/applicant/SortApplicantParser.java
0.00% <0.00%> (ø)

src/main/java/seedu/address/model/AddressBook.java
86.95% <0.00%> (-2.60%)
:arrow_down:

src/main/java/seedu/address/model/Model.java
100.00% <ø> (ø)

...rc/main/java/seedu/address/model/ModelManager.java
84.09% <0.00%> (-1.96%)
:arrow_down:

...ava/seedu/address/model/applicant/DateApplied.java
75.00% <0.00%> (-6.82%)
:arrow_down:

...a/seedu/address/model/applicant/InterviewDate.java
51.72% <0.00%> (-10.78%)
:arrow_down:

... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e284ff...f9fa4b2. Read the comment docs.


Add 'spo group user add' command. Closes #2483

Create a new Command 'spo group user add' for adding the user to SharePoint Group
Adds 'spo group user add' command for adding the user to SharePoint Group. Closes #2483

Linked Issue

Closes #2483

@pnp/cli-for-microsoft-365-maintainers  - Just an FYI about the implementation. For adding the users to SharePoint group, we have used the endpoint, /_api/SP.Web.ShareObject rather than the conventional endpoint, _api/web/SiteGroups/GetById(...)/Users/AddUserById(...).
The reason is because, our command gives the feature to add multiple users to the group. If we go with the latter endpoint, it does not support the addition of multiple users in the single call, while with former, you can add multiple users in the single command and our code does not need to handle looping mechanism.
Let me know if you see any challenges in the approach.

If it works, it works 👏

We're almost there. Let's do a few adjustments before we merge it.

Thanks Waldek for the detailed review. It really shows that I have become rusty in CLI 🙄. Post the code review  is the best part of me with your valuable comments. Thanks for being patient in reviewing.
Will look into the comments and update you.

No worries, we're in this together 👍

Thanks! Let's do another pass 👍

Hey Waldek - I have made couple of changes in the following PR.

Validation of active users before initiating adding to the groups
For this. we have used the consumption of existing command as per this conversation.
Removed interfaces which are not used for the functionality.

Like always looking forward your valuable feedback.

Ready to initiate PR. Since there is lack of 100% coverage for the main branch as per #2586 , keeping this on hold. Once that is complete, will initiate the PR after rebasing the fix.

Marking the PR Ready for Review since #2586 is already closed.

When trying to add a valid user, I'm getting an error: Error: Couldn't resolve the users.. The API call that's failing is:

Ahh that is a scenario which I had not foreseen. Sorry that you had review again without success. Silly, but huge mistake. 🤦♂
Thanks for finding the root cause too. 👍
I was using the command spo user get for validating the user and for that user to be part of the User Information List. But in this case, user just need to be part of AAD. Hence, I have changed the command used to be aad user get which will ensure that user do exists.
I had implemented the changes and had tried various test cases to ensure that we are not missing anything. I will be initiating PR shortly with the changes.
Can you let me know if I am overlooking anything else.

No worries, that's why we check each other's work to ensure that we haven't missed anything. Thank you for a quick turnaround 👏

Thanks @garrytrinder for the review. It is definitely a case where you will get a false positive if in actual outcome is partial success..
But I was not sure how did we show it across to the end users.
Do you envision to  show the failure users as a different message along with the added users? If that is the case, we may have to alter the default output object of the API call.
I cannot think of other commands where we can have partial success in the single command. Any thoughts on that please.?

I cannot think of other commands where we can have partial success in the single command. Any thoughts on that please.?

A similar command to this would be aad o365group set which accepts multiple owners, looking at its implementation just appear to be validating that each user string is a valid format.
https://github.com/pnp/cli-microsoft365/blob/928c2ed0b4a6c6f7771178de7f52ac511a6bc411/src/m365/aad/commands/o365group/o365group-set.ts#L262-L269
However, the same test passing, i.e. an incorrect user, results in no error message being displayed.

As your command exhibits the same behaviour, shall we create a discussion to continue this conversation separate to this PR, we can then get this merged and come back to the error handling as a feature enhancement?

Thanks Garry for looking into that. I guess handling partial success scenarios for multiple values are not quite common across.
In parallel, I had checked some of the PowerShell commands and somehow I could not find commands which takes care of this kind of scenarios.
Good idea to create a discussion and we can brainstorm and decide and can update the commands accordingly.

Merged manually 👍


Solution of "FileNotFoundError: no weights file found in pretrained_weights\stable-diffusion-v1-5\unet"

Solution of "FileNotFoundError: no weights file found in pretrained_weights\stable-diffusion-v1-5\unet"
Add to './src/models/unet_3d.py'
pretrained_model_path = pretrained_model_path.joinpath(SAFETENSORS_WEIGHTS_NAME)
before line 646
        if str(pretrained_model_path).endswith(".safetensors"):
            logger.debug(
                f"loading safeTensors weights from {pretrained_model_path} ..."
            )
            state_dict = load_file(
                pretrained_model_path, device="cpu"
            )

Just to be more specific as I had the same problem, I fixed it by putting this safetensor into the folder mentioned above... (pretrained_weights\stable-diffusion-v1-5\unet)
https://civitai.com/models/155256/stable-diffusion-v15-bf16fp16-no-emaema-only-no-vae-safetensors-checkpoint
I then added the line mentioned and included the name of the new checkpoint...
pretrained_model_path = pretrained_model_path.joinpath("stableDiffusionV15Bf16Fp16NoEma_v15NoEma.safetensors")
That got it working


VG-5571 - search bbox color

Codecov Report

Merging #121 into master will increase coverage by <.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   79.77%   79.78%   +<.01%
==========================================
  Files         117      117
  Lines        7690     7692       +2
  Branches     1158     1158
==========================================
+ Hits         6135     6137       +2
  Misses       1555     1555

Impacted Files
Coverage Δ

app/src/search/search-map2.js
75.67% <83.33%> (+0.13%)
:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab90ad3...d47fa7c. Read the comment docs.


Filter box can't get existing values when templating applied on time range field

For some of our use cases, we must include templating values from the selected time range when defining our virtual dataset. Eg, use {{ from_dttm}} and {{to_dttm}}. Then we create a chart from this dataset and include it in a dashboard. The functionality of templating works and the chart changes correctly when applying different date ranges.
However, when adding filters on other columns within the dataset, The filter box cannot get existing values.
How to reproduce the bug

define a test dataset with following sql:

with example_data as (
SELECT 1 as col1, 'a' as col2, to_date('20221001','yyyyMMdd') as dt
union
SELECT 2 as col1, 'b' as col2, to_date('20221002','yyyyMMdd') as dt
union
SELECT 3 as col1, 'c' as col2, to_date('20221003','yyyyMMdd') as dt
)
select * from example_data
where dt  >= '{{ from_dttm }}'
    AND dt < '{{ to_dttm }}'

create a simple chart with configs in the picture , using dataset from step1

create a dashboard with the chart in step 2 and add some filters on the columns

Expected results
The filters on the left side should contain some values

Actual results
The filters on the left side does not show any value

Environment
(please complete the following information):

browser type and version: Version 107.0.5304.87 (Official Build) (x86_64)
superset version: superset:2.0.0
python version: python 3.8

Working as Expected in the recent release.

Working as Expected Results. We can able to see the filter values.
Screenshots:-
1.Without applying filter values

2.After applying filter values

Filter Box has been deprecated and is removed in (forthcoming) Superset 4.0. Closing this and other filter box issues.


Add Rate Limiting

Rate limiting is used to control how many requests a given consumer can send to the API.
To tell your API users how many requests they have left, set the following headers:
X-Rate-Limit-Limit, the number of requests allowed in a given time interval
X-Rate-Limit-Remaining, the number of requests remaining in the same interval,
X-Rate-Limit-Reset, the time when the rate limit will be reset.

https://www.geeksforgeeks.org/what-is-express-rate-limit-in-node-js/

Rate Limit
Requests through the API are rate limited per IP Location. Current rate limits:
1,000 requests per hour
50 requests per minute (5% of the hourly total)
Once you exceed either limit, you will be rate limited until the next cycle starts. Space out any requests that you would otherwise issue in bursts for the best results.
The rate limiting information is contained within the response headers of each request. The relevant headers are:
RateLimit-Limit: The number of requests that can be made per hour.
RateLimit-Remaining: The number of requests that remain before you hit your request limit. See the information below for how the request limits expire.
RateLimit-Reset: This represents the time when the oldest request will expire. The value is given in Unix epoch time. See below for more information about how request limits expire.
As long as the RateLimit-Remaining count is above zero, you will be able to make additional requests.
The way that a request expires and is removed from the current limit count is important to understand. Rather than counting all of the requests for an hour and resetting the RateLimit-Remaining value at the end of the hour, each request instead has its own timer.
This means that each request contributes toward the RateLimit-Remaining count for one complete hour after the request is made. When that request's timer runs out, it is no longer counted towards the request limit.
This has implications on the meaning of the RateLimit-Reset header as well. Because the entire rate limit is not reset at one time, the value of this header is set to the time when the oldest request will expire.
Keep this in mind if you see your RateLimit-Reset value change, but not move an entire hour into the future.
If the RateLimit-Remaining reaches zero, subsequent requests will receive a 429 error code until the request reset has been reached. You can see the format of the response in the examples.
Sample Rate Limit Headers
    RateLimit-Limit: 1200
    RateLimit-Remaining: 1193
    RateLimit-Reset:<PHONE_NUMBER>

Sample Rate Exceeded Response
    429 Too Many Requests
    {
            id: "too_many_requests",
            message: "API Rate limit exceeded."
    }

Hi @ageddesi, Please assign me this task; I'd like to do it.

Hi @Vishwamagdavan that would be great thank you. Please can you make sure the limits can be configured via the .env file. 👍 Good luck, if you need any help message me here or join us on discord. https://discord.gg/WEtvwvQ7


Get old deleted/edited messages

General Troubleshooting
Hey there! Before you report a bug or suggest a new feature,
please make sure to follow these steps first!

[x] I have checked the branches or the maintainers' PRs for upcoming features/bug fixes.
[x] I have checked for similar issues.
[x] I have updated to the latest JDA version.

This is not the place to learn Java. Please refer to StackOverflow
for your general programming questions.

Template
Issue Type

[ ] Bug Report
[x] Feature Request

Description
MessageDeleteEvent and  MessageUpdateEvent should definitely have methods to get the old deleted/edited message, as of now, there is nothing (as far as I know) that does that.

JDA cannot provide information that discord doesn't provide.

You will have to cache messages yourself if you need to have such features.
Discord itself doesn't provide that via event and we don't cache it because it would just unnecessarily fill up ram no matter if used or not, while our other caches (users, guilds, channels) are mandatory for JDA to work.

Could anybody give me advice on how to cache the messages?


Consistent Unix LF line endings

Line endings produce warnings when performing certain operations with Git because a select few of the committed files have CRLF line endings, while most have LF in the repo. Let's normalize the outliers to LF since Git can nicely transform them to CRLF if needed with git config core.autocrlf auto for CRLF-desiring folks.

@DmitryBaranovskiy Any thoughts about merging this PR?  The line endings are an important tune-up.

This has been open too many years, so I'm abandoning hope it will be merged and helping tidy the PR queue by closing it.


Changed Lavalink docs

Changelog

Changed $lavalinkExecute page
Added Lavalink.md guides in advanced-guides

This commit should be the final


Docker facade never healthy

Bug description
Request createWorkspace failed with message: 13 INTERNAL: cannot resolve workspace image: failed to do request: Head "https://registry.gitpod.spicpod.tech/v2/workspace-images/manifests/2e37fc8e3d3884eca309093358f96b5062dbee61df27337b4bf41e0d18ac78e0": dial tcp: lookup registry.gitpod.spicpod.tech on <IP_ADDRESS>:53: no such host

Unknown Error: { "code": -32603 }

Steps to reproduce
if i told you this was the last issue, it'd probably be a lie, but its close, trying to launch a workspace but the registry seems to be sad and gitpod ui responds saying there's on such host.
annnnnny clue at all? is it supposed to be an internal loop back or is it really supposed to route through the loadbalancer? I can't tell
Expected behavior
shouldn't crash.
Example repository
No response
Anything else?
No response

It looks like something to do with the load balancer
 dial tcp: lookup registry.gitpod.spicpod.tech on <IP_ADDRESS>:53: no such host

Questions to check:

Have you got gitpod.spicpod.tech, *.gitpod.spicpod.tech and *.ws.gitpod.spicpod.tech set up in the DNS? They should all point through to the same A/CNAME record (I'm getting a fail when I do an nslookup, so I think it's this)
Have you got the TLS cert set on the same three domains?

What a knob, I had "fix the DNS properly before handing the terraform over for testing" on my list 😫

I know that pain greatly

I'm loving the bugs that are getting closed themselves 😄

Part of me says I shouldn't file them, then the other part says if other people go through this and hit similar stuff, at least there's some trail of moronic actions.

Yeah, you definitely should. This -32603 message is the stupidest/most unhelpful one ever. It was the issue we'd have raised the most often, simply because the nice error message doesn't bubble up to the webpage


Add translations for greet and code

Examples:
My dog likes to greet me at the door. : My dog likes to ping me at the door.
I like to code.: I like to speak machine.

Thanks @monicahung! 👍


Add suffix= argument to to_parquet (and writers)

Adds a simple feature allowing the user to override the default ".parquet" and ".data" file extensions for parquet and hugectr output, respectively.  The primary modivation was the Dataset.to_parquet method - That method currently allows the the user to specify preserve_files=True, but there is no way for them to specify if the file extension should be changed (and how).  With this PR, the extension will be replaced with the value of suffix (defaulting to ".parquet" for parquet), unless the user has already specified explicit names with the output_files argument.
Note: This feature simplifies conversion code in #672 a bit.

rerun tests


Add (un)stake all_hotkeys and bittensor.Balance

Pull Request Test Coverage Report for Build ce128e95-95be-407e-bad6-d734bf1222b7

75 of 114 (65.79%)  changed or added relevant lines in 2 files are covered.
8 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.2%) to 81.681%

Changes Missing Coverage
Covered Lines
Changed/Added Lines
%

bittensor/utils/balance.py
72
111
64.86%

Files with Coverage Reduction
New Missed Lines
%

bittensor/_serializer/serializer_impl.py
1
82.26%

bittensor/_subtensor/subtensor_impl.py
1
79.62%

bittensor/_dataset/dataset_impl.py
6
90.03%

Totals

Change from base Build aacd5805-73b5-4d65-8bd9-09d433941287:
-0.2%

Covered Lines:
4218

Relevant Lines:
5164

💛  - Coveralls


Add .travis.yml

Since the plugin is enabled for pull requests. Otherwise all PRs will be marked with a failing build.
Could not find .travis.yml, using standard configuration.

Would make more sense to either disable travis or add tests. Volunteers for writing tests are welcome :)
I don't think I can disable Travis on this repo. @StefanKarpinski  could you do that?

@carlobaldassi I'm not sure I've ever seen anyone write tests for vimscript. I like the idea of disabling Travis if it's not helping though.

I have just disabled travis for this repository. Writing tests for vimscript would be possible in principle but it's a major pain (volunteers welcome).
(@tkelman thanks for granting me access!)


Ubely/pre_sbuild v4 1

A continuation of #257

This patchset is compatible with upcoming sbuild patchset, where some 'export' related things were added.
Briefly, there is an environment comparison between do_prepare_build and dpkg_runbuild tasks is done, so any variable exported in dpkg_runbuild_prepend goes to sbuild environment.
So, there is no need to have one more patch (export => dpkg_build_export) like it was done in https://github.com/siemens/meta-iot2050/pull/252


MNT: Accuracy test projection to take advantage of matrix transform to yield more accurate results

One change to consider

NOTE: Also investigate the random generation bound

Also please set the focal lengths to values that correspond to some sensible field of view. For 40 degrees (x) and 30 degrees (y) we calculated focal lengths 879.19 and 895.69, respectively.

Focal length and the board generation have been fixed. I will try to investigate the high RMS planar.


SR-20 Update swift --help to explain multitool support ("swift build"…, etc.)

Resolves SR-20.

@swift-ci Please smoke test

yay, thanks!

Thanks, @kelhutch17; welcome to the project!


Trust banner: appears a bit hectic

It seems to me that opening an empty window with a file first opens the file and then renders the banner. Can we not eagerly show the banner on startup when we know that we open an editor?
Now it feels like something is pushing down the UI after I had opened VSCode:

Currently we optimize for the trusted flow which does not show the banner. When in restricted mode, we want to avoid slowing down the rest of the workbench so we do not delay anything just to wait for restricted mode (trust status, which takes some time to fully calculate). The current restricted mode flow IMO optimizes for perceived performance.


[Data Import] As a developer I'd like to be able to import files of data...

...so that I can set up the application with the necessary data for the administrator to work with.
This would most likely apply to users or policies.

is implemented, PR reference: https://github.com/nearform/labs-authorization/pull/191


silence warnings

Hi,
I'm getting boring warning mails from ROS buildfarm. This should silence them.

Hi @nim65s , thanks for the fix, I was also getting the annoying warnings from the build farm. It's not clear to me if @jcarpent  and you have converged to an agreement. Let me know if you want me to merge this to devel.


Taxonomy Column Crawled Properties and auto generated Managed properties suddenly dissappeared in all tenats

Category

[ ] Question
[ ] Typo
[x] Bug
[ ] Additional article idea

In all of our SharePoint Online tenants crawled properties and auto generated managed properties for taxonomy columns suddenly disappeared on 2018-08-03.

Only the SharePoint default taxonomy crawled properties are availabe anymore.
The search is activated for the lists and the site collections where the columns exist. Reindexing the list does not seem to have any effect.
Expected or Desired Behavior
After creating a new taxonomy column in a list a new crawled property should appear after the list has been indexed by the search. Furthermore a new managed property should be auto generated for this columns.
Observed Behavior
All taxonomy column crawled properties and auto generated managed properties have disappeared.
Edit: The properties have reappeared over the weekend but the managed properties have no crawled properties mappings anymore:

Looks like it is time for a support ticket.

Microsoft Support helped us with this problem. The problem occured because invalid Data was written in the Managed Metadata Note field of listitem. This caused SharePoint search not to crawl the items at all.


Nested raw queries do not handle identifier bindings correctly

If we attempt to use knex.raw to write the following type of query:
> select * from (select 'val' as "name?") as t
 name?
---------
 val
(1 row)

by writing the following...
knex.raw('select * from (?) as t', [knex.raw('select ? as ??', ['val', 'name?'])])
    .catch(e => console.error(e))

we get this error:
{ error: 'select * from (select $1 as "name$2") as t -
     could not determine data type of parameter $1' }

This is because knex eagerly applies identifier bindings when evaulating nested raws.
If a similar strategy could be applied as with value bindings (concatenating value parameters to build up a list of all bound parameters for the concatenated SQL string), it should be possible to support this.
The current flow is:

root raw toSql
nested raw toSQL
nested raw applies identifier bindings (e.g named? is placed into the SQL string)
root raw include the SQL from nested raw within its own SQL, and then scans this string for placeholders. Since the nested raw has already included its identifier values, it identifies them as parameters
our final SQL is therefore select * from (select ? as "name?") as t, which is then scanned for placeholder values and we get the behaviour above

Ideally it'd:

root raw toSql
nested raw toSQL
nested raw captures all identifier bindings, and returns this array
root raw creates a list of all identifier bindings, and applies them in one go (e.g final SQL/bindings for our example would be select * from (select ? as ??) as t { values: ['val'], identifiers: ['named?' }.

Actually using ? -mark in question in identifiers breaks even without nesting. You could try to replace ? in identifier with \? as a workaround.
Anyways, I like the idea of just collecting identifiers as they were given and interpolate them only in the very end of the query compilation phase. But I would actually also like it better if toSQL result query would already be in format accepted by underlying driver...
Both would be big changes though and I would like to have opinions of other collaborators before anyone would start working on these.

You're quite right re ? being an issue even without nesting. Thanks!

I think we can say that one should not use identifiers containing ? or . characters when using knex (pretty marginal case).

I'm trying to use a REGEXP_REPLACE('column', '({{(@root\.)?app\.url}})(?!#)', '\1replace_thing') I need the question marks in the regex, are there any solution on how to escape these ?

Pass regexp as value binding and not as a static string embedded in sql.

@elhigu escaping ? works, but escaping . still gives the issue. What's your suggested workaround?

Dont use periods or dont use ?? binding in that case. ?? bindings treats periods as special character and that is not going to change.

thanks elhigu! I will find a workaround to decorate those columns after knex has been used!

@elhigu what's the easiest way for me to find out which characters I should avoid when using ???

safest to avoid everything that is not a-zA-Z0-9_-. that binding should be used for 'table.column' type of references.


This works very well

Thanks

There could be a missing file.exists(). So don't change name or delete your selected file. If you find program doesn't start, try app settings delete app data.
Here are two test lines for your file:
#EXTINF:-1 tvg-id="bunny" tvg-name="bunny" tvg-logo="https://www.wowza.com/html/images/bigbuckposter.jpg" group-title="Test;test",Bunny
rtsp://wowzaec2demo.streamlock.net/vod/mp4:BigBuckBunny_115k.mov
If you don't wish program to retrieve logo, disable access to network in your filewall.
MPV works well...

.. If you type a web address.m3u into your browser, you might find  it just plays.. Then in the browser select "add to home screen". Then, widget created that with a tap, plays.
[Hope this helps]


Added Devious Minds quest.

Let me know if anything needs to be changed :)

Going to withrdaw so I can make some changes (GeneralRequirements for example) and to reduce the commit to just the Devious Minds quest (Haven't really used git before this so been a learning experience).

Going to withrdaw so I can make some changes (GeneralRequirements for example) and to reduce the commit to just the Devious Minds quest (Haven't really used git before this so been a learning experience).


fix shcluster boostrapping

needs disabled: 0 in shclustering conf in group vars.. separate pr..

TASK [group/shcmember : Run init shcluster-config] *****************************
fatal: [<IP_ADDRESS>]: FAILED! => {"changed": true, "cmd": ["/opt/splunk/bin/splunk", "init", "shcluster-config", "-mgmt_uri", "https://<IP_ADDRESS>:8089", "-replication_port", "9888", "-auth", "admin:pickles"], "delta": "0:00:01.154473", "end": "2016-03-07 16:58:08.700312", "failed": true, "rc": 11, "start": "2016-03-07 16:58:07.545839", "stderr": "This command [POST /services/shcluster/config/config] needs splunkd to be up, and splunkd is down.", "stdout": "", "stdout_lines": [], "warnings": []}
fatal: [<IP_ADDRESS>]: FAILED! => {"changed": true, "cmd": ["/opt/splunk/bin/splunk", "init", "shcluster-config", "-mgmt_uri", "https://<IP_ADDRESS>:8089", "-replication_port", "9888", "-auth", "admin:pickles"], "delta": "0:00:01.242790", "end": "2016-03-07 16:58:08.982815", "failed": true, "rc": 11, "start": "2016-03-07 16:58:07.740025", "stderr": "This command [POST /services/shcluster/config/config] needs splunkd to be up, and splunkd is down.", "stdout": "", "stdout_lines": [], "warnings": []}


Time since last town hall

Visitors would like to know the amount of time that has passed since their MoC has held a time hall.

this is a duplicate


Infinite spinner when browsing Tasks tab

Hey there.
Faced subj in 0.19.2. Either navigate from Status -> Scheduled or directly from Tasks -> Scheduled just shows infinite spinner.
Tasks -> Active is working fine. Please ping me for more details.
Any possible solutions? Thanks.

Have you checked the console log for errors? It's likely an api call failed or something similar, so it didn't get past the loading icon. Any other details on the error that occurred would be helpful

This seems to be a UI/JS error.
Its looking for a property that is non existent. I have uploaded a screenshot below.
This happens with the latest (0.20.0) singularity as well. Can someone please confirm if this is an error?

Image above.

Confirmed for 0.20.0

Fixed in 0.20.1


HW6 Part 2 fk2421 added npmjs, updated proj desc

Ran the commands regarding markdown lint but still received line length related error


Better UX for location and timezone filter accordion

This PR improves the UX for both location and time zone accordions. The accordion should be kept open when the user selects from those filters.
Related issue: https://github.com/joemasilotti/railsdevs.com/issues/558
Pull request checklist

[x] My code contains tests covering the code I modified
[x] I linted and tested the project with bin/check
[ ] I added significant changes and product updates to the changelog

Hey @ypcethan, do you still have appetite to finish this PR? It's been about a month since your last commit.
If not, no worries! Just let me know and I'll take it over from here.


Implement WASI FS fd_filestat_get

This is the proper version of #836
Things that have not been fully implemented of __wasi_filestat_t:

dev: device, id of device, don't know if we can get this from somewhere?
ino: inode, file serial number, don't know if we can get this from somewhere?
nlink: linkcount, number of hard links to the file, don't know if we can get this from somewhere?
atim: timestamp, last data access timestamp, we just have the mod time so I made it use that, maybe we should leave this 0?
ctim: timestamp, last file status change timestamp, we just have the mod time so I made it use that, maybe we should leave this 0?

@jerbob92 what you've done looks like a good start. I would move the questions in your PR desc to comments in the impl as this being a virtual filesystem and wasi being underspecified.. there aren't perfect choices. Later, we'll have to add a
next steps would be to add unit test similar to the existing ones, and also vet through a couple compilers that it works. The latter might be easier as so many use wasi-libc.
Maybe try tinygo rust or zig as they are already in the example project.  This will help ensure that the partial implementation is usable (often is!) What I mean is you can play around in imports/wasi_snapshot_preview1/example/testdata easier as it has Makefile targets to compile the sources. This may help in ad-hoc testing to ensure it works'ish. I don't mean to change the cat example, just use the infra there to ensure the implementation is usable.
Thanks!

@codefromthecrypt done, let me know what you think :)

I plan to contribute to polishing this tomorrow or earlier

rebased

@jerbob92 looks good to me. once you ack I'll merge

Ack


request and set the csrf header protection added to brooklyn server

Minor tweaks to opt-in to CSRF protection in Brooklyn UI
NB requires https://github.com/apache/brooklyn-server/pull/430
(actually it will work fine without that, but it will be no-op)

+1 looks good to me to the best of my knowledge.  See comment on apache/brooklyn-server#430.

@ahgittin this breaks logout button :(

@bostko oh no - pretty sure i tried that but will check

I didn't check it unfortunately - tried other things but don't normally hit the logout button.

Sorry for disturbing you I think I am wrong that this exactly broke it.
Please accept that as a reminder to test logout.
Logout htaccess can be broken in numerous ways...

@bostko what is the problem you're encountering?
i've just tested, running BrooklynJavascriptGuiLauncher with this in my brooklyn.properties:
brooklyn.webconsole.security.users=admin
brooklyn.webconsole.security.user.admin.password=nimda
brooklyn.webconsole.security.provider=org.apache.brooklyn.rest.security.provider.ExplicitUsersSecurityProvider

logout seems to work fine in every permutation i've tried.  note there were some changes to LogoutResource in https://github.com/apache/brooklyn-server/pull/430 -- i did my testing with them ... they're now in master so shouldn't the issue but might be?

i see, the problem is the swagger page, @bostko you're addressing in https://github.com/apache/brooklyn-ui/pull/36 -- commented there
(in general logout is working okay and this PR we think is fine)


Question: Can we support enum-typed fields?

From the README:

Enumerations
All .proto enumeration types convert to the Rust i32 type. Additionally,
each enumeration type gets a corresponding Rust enum type, with helper methods
to convert i32 values to the enum type. The enum type isn't used directly as
a field, because the Protobuf spec mandates that enumerations values are 'open',
and decoding unrecognized enumeration values must be possible.

Given an enum:
enum Foo {
    A = 1;
    B = 2;
}

Would it be possible to support "open" enums with a special Unknown(i32) variant like this?
enum Foo {
    A,
    B,
    Unknown(i32),
}

impl From<i32> for Foo {
    fn from(i: i32) -> Self {
        match i {
            1 => Foo::A,
            2 => Foo::B,
            _ => Foo::Unknown(i),
        }
    }
}

impl From<&Foo> for i32 {
    fn from(f: &Foo) -> Self {
        match f {
            Foo::A => 1,
            Foo::B => 2,
            Foo::Unknown(v) => *v,
        }
    }
}

That would enable enum-typed fields while allowing match expressions to naturally handle unknown values.

Possible duplicate of #276

That would enable enum-typed fields while allowing match expressions to naturally handle unknown values.

There's an existing way to do. this: use Foo::from_i32, which returns None if the i32 is not a known variant.

prost predates the TryFrom API, which is why it's a method.  We should probably fix that.

Here's my solution:
use num_derive derive all enums :  #[derive(FromPrimitive, ToPrimitive)].
But you must handwrite all enums in bulid script which is inconvenient.

This would be very convenient when trying to mix Prost with serde. GRPC enums are represented as strings. For example, a failed request might look like:
{
  "error": {
    "code": 400,
    "message": "API key not valid. Please pass a valid API key.",
    "status": "INVALID_ARGUMENT",
  }
}

Where status is the well known type defined here: https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto
(see here for more details on error mapping: https://cloud.google.com/apis/design/errors#http_mapping)
As is, a custom serde deserializer function needs to be implemented for each enum. If prost used a native enum type, all that would be required is a #[serde(rename_all = "SCREAMING_SNAKE_CASE"] annotation


OCPBUGS-23083: adding permission to CNO RBAC Calico path for network-node-identity deploy

Allow the cluster network operator to create network-node-identity deployment in Calico environment
Fixes https://issues.redhat.com/browse/OCPBUGS-23083
Checklist

[x] Subject and description added to both, commit and PR.
[ ] Relevant issues have been referenced.
[ ] This change includes docs.
[ ] This change includes unit tests.

/approve

/jira refresh

/cherry-pick release-4.14