Move back to ChannelMonitor RemoteTxCache #610

ariard · 2020-04-28T00:25:17Z

#561 & consorts were moving some remote tx data in OnchainTxHandler, after a lot of back-and-forth on API between ChannelMonitor and OnchainTxHandler, it has been laid out that OnchainTxHandler should be scoped to transaction-finalization and fee-bumping, transaction construction or templating should be provided by ChannelMonitor. Therefore, RemoteTxCache, as channel dynamic data belong to ChannelMonitor.

TheBlueMatt

Just looked over the full diff pretty quickly, but this looks pretty nice - not much diff for a lot of functionality :). Some comments, mostly simple docs/naming/etc stuff, but this is looking good :).

TheBlueMatt · 2020-04-29T04:07:34Z

lightning/src/chain/keysinterface.rs

@@ -245,12 +247,24 @@ pub trait ChannelKeys : Send+Clone {
 	/// return value must contain a signature.
 	fn sign_local_commitment_htlc_transactions<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, local_csv: u16, secp_ctx: &Secp256k1<T>) -> Result<Vec<Option<Signature>>, ()>;

+	/// Signs a justice transaction.


These need real documentation, a number of the parameter names don't appear to be correct, etc. But I'm sure you knew that :)

Yeah I knew it. But was expecting better documentation to be part of your key-interface cleanup but it got merged first ;)

TheBlueMatt · 2020-04-29T04:08:05Z

lightning/src/ln/chan_utils.rs

@@ -198,7 +198,7 @@ pub(super) fn derive_public_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>,
 /// Derives a revocation key from its constituent parts.
 /// Note that this is infallible iff we trust that at least one of the two input keys are randomly
 /// generated (ie our own).
-pub(super) fn derive_private_revocation_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_secret: &SecretKey, revocation_base_secret: &SecretKey) -> Result<SecretKey, secp256k1::Error> {
+pub fn derive_private_revocation_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_secret: &SecretKey, revocation_base_secret: &SecretKey) -> Result<SecretKey, secp256k1::Error> {


The docs here should likely be improved if we're making it pub.

pub (crate) fn is enough for now

Strongly disagree - you can't easily implement a ChannelKeys without it. Lets just write a comment :).

lightning/src/ln/channelmonitor.rs

TheBlueMatt · 2020-04-29T04:09:42Z

lightning/src/ln/onchaintx.rs

-#[derive(PartialEq)]
-pub(super) enum InputDescriptors {
+#[derive(PartialEq, Clone, Copy)]
+pub(crate) enum InputDescriptors {


Should we be dropping the "s"? This doesn't seem to be plural, which I find confusing.

Planning to move InputDescriptors to better-documented/generic TxTemplate as described in #606

TheBlueMatt · 2020-04-29T04:10:33Z

lightning/src/ln/channelmonitor.rs

@@ -823,13 +842,10 @@ impl<ChanSigner: ChannelKeys> PartialEq for ChannelMonitor<ChanSigner> {
 			self.funding_info != other.funding_info ||
 			self.current_remote_commitment_txid != other.current_remote_commitment_txid ||
 			self.prev_remote_commitment_txid != other.prev_remote_commitment_txid ||
-			self.their_htlc_base_key != other.their_htlc_base_key ||


Please add the new fields?

Damn I new I forgot it at some point but never committed... Corrected

TheBlueMatt · 2020-04-29T04:13:11Z

lightning/src/ln/channelmonitor.rs

-			their_htlc_base_key: &PublicKey, their_delayed_payment_base_key: &PublicKey,
-			their_to_self_delay: u16, funding_redeemscript: Script, channel_value_satoshis: u64,
+			remote_htlc_base_key: &PublicKey, remote_delayed_payment_base_key: &PublicKey,
+			on_remote_csv: u16, funding_redeemscript: Script, channel_value_satoshis: u64,


Hmm, on_remote_csv definitely makes more sense than their_to_self_delay, but maybe call it remote_tx_to_remote_csv, or so? Characters don't cost us much :)

on_remote_tx_csv ? Imo remote_tx_to_remote_csv doesn't make sense because even if are processing a remote tx we are doing so from the viewpoint of local, this csv matters us as local.

Overall, either is fine but to avoid confusion, it's more important to be consistent on this with every variable names.

I'm ok with that, though I prefer something that indicates that its the CSV on the output to our counterparty on a remote tx.

lightning/src/ln/channelmonitor.rs

TheBlueMatt · 2020-04-29T23:33:47Z

Needs rebase on master.

ariard · 2020-04-30T01:26:17Z

Rebased at c7761cf, with more renaming commits for killing their_to_self/our_to_self in ChannelMonitor, confusion by their names was reason of test failure.

codecov · 2020-04-30T02:27:34Z

Codecov Report

Merging #610 into master will increase coverage by 0.03%.
The diff coverage is 95.32%.

@@            Coverage Diff             @@
##           master     #610      +/-   ##
==========================================
+ Coverage   91.29%   91.32%   +0.03%     
==========================================
  Files          35       35              
  Lines       20776    20952     +176     
==========================================
+ Hits        18967    19134     +167     
- Misses       1809     1818       +9

Impacted Files	Coverage Δ
lightning/src/ln/onchaintx.rs	`93.94% <90.32%> (-0.91%)`	⬇️
lightning/src/chain/keysinterface.rs	`94.86% <91.17%> (-2.11%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.71% <98.48%> (+0.20%)`	⬆️
lightning/src/ln/chan_utils.rs	`97.17% <100.00%> (ø)`
lightning/src/ln/channel.rs	`86.63% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.43% <100.00%> (+0.11%)`	⬆️
lightning/src/util/enforcing_trait_impls.rs	`100.00% <100.00%> (ø)`
lightning/src/util/macro_logger.rs	`89.28% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`78.21% <100.00%> (+0.37%)`	⬆️
lightning/src/ln/reorg_tests.rs	`98.94% <0.00%> (-0.04%)`	⬇️
... and 1 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 d4ad57b...81e358c. Read the comment docs.

lightning/src/chain/keysinterface.rs

ariard · 2020-05-01T02:44:38Z

Updated 9eb0620, see IRC conv, I would prefer to move forward with this and address witnessScript dry-up in a follow-up. This PR is already big enough and touch to some critical path.

TheBlueMatt · 2020-05-02T19:44:54Z

see IRC conv, I would prefer to move forward with this and address witnessScript dry-up in a follow-up. This PR is already big enough

Not sure which IRC conversation this is in reference to, but I don't get why - such a change does not change the size of this PR at all and its a comment specific to the new code, not something related to cleaning up old code, so I don't think this is a reasonable excuse.

ariard · 2020-05-04T06:42:17Z

Not sure which IRC conversation this is in reference to, but I don't get why - such a change does not change the size of this PR at all and its a comment specific to the new code, not something related to cleaning up old code, so I don't think this is a reasonable excuse.

Okay updated at cf18536, turned out every element was available and didn't have to touch to ChannelMonitor.

lightning/src/chain/keysinterface.rs

TheBlueMatt · 2020-05-04T16:41:05Z

lightning/src/chain/keysinterface.rs

@@ -64,8 +65,8 @@ pub enum SpendableOutputDescriptor {
 	DynamicOutputP2WSH {
 		/// The outpoint which is spendable
 		outpoint: OutPoint,
-		/// The secret key which must be used to sign the spending transaction
-		key: SecretKey,
+		/// Per commitment point to derive delayed_payment_key by key holder


derive how? Please point the user to ln::chan_utils::derive_private_key, make it pub, and describe exactly which inputs should be passed to it. Based on the docs here I don't know how to sign for this transaction, whereas previously I easily could. Note that the docs also, above, say "The private key which should be used to sign the transaction is provided" which is also wrong now.

Done, also made public get_revokable_redeemscript, you do want to regenerate script for verification.

TheBlueMatt · 2020-05-04T16:43:22Z

lightning/src/ln/functional_tests.rs

@@ -4088,7 +4088,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() {
 }

 macro_rules! check_spendable_outputs {
-	($node: expr, $der_idx: expr) => {
+	($node: expr, $der_idx: expr, $chan_signer: expr) => {


Hmmmm...This is not ok - the point of this check is to check that the user can sign for outputs we notify them of, not that we can sign for outputs we notify them of. Making them dig into private fields of the ChannelMonitor to get the chan_signer back and then call methods on it is not ok.

Okay changed to TestKeysInterface::get_descriptor_base_keys()

TheBlueMatt · 2020-05-04T16:44:20Z

lightning/src/chain/keysinterface.rs

+	///
+	/// Per_commitment_point is the dynamic point corresponding to the channel state
+	/// detected onchain.
+	fn sign_payment_transaction<T: secp256k1::Signing>(&self, spend_tx: &Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey, network: Network, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;


I'm confused - we never call this outside of tests, why is it in a public interface?

TheBlueMatt · 2020-05-04T17:20:45Z

lightning/src/chain/keysinterface.rs

+	/// Per_commitment_point is the dynamic point corresponding to the channel state
+	/// detected onchain. It has been generated by us and is used to derive our
+	/// delayed_payment_key.
+	fn sign_delayed_transaction<T: secp256k1::Signing + secp256k1::Verification>(&self, spend_tx: &Transaction, input: usize, on_local_tx_csv: u16, amount: u64, per_commitment_point: &PublicKey, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;


Just in general I think this is the wrong way to go about it - removing the secret key is great, but there's no reason to require that a ChannelKeys has this function - the user can chose to sign the delayed transaction any way they want and we never actually need it. We should obviously provide a way to get at the relevant keys from InMemoryChannelKeys so that the user can sign correctly, but if its eg a hardware wallet, its not our job to tell them how to implement the API.

Yes should add somewhere keys don't have to reside on same device.

TheBlueMatt · 2020-05-08T16:58:25Z

lightning/src/chain/keysinterface.rs

-	/// confirms).
+	/// <BIP 143 signature> <empty vector> (MINIMALIF standard rule) <provided witnessScript>
+	///
+	/// Note that the nSequence field in the spending transaction input must be set


Why did you chane this? the previous copy "mentions in the input" which you removed, but sequence is per-input. Also note that both the old and the new copy are missing a "to" before "to_self_delay.

I modified back to "spending input", IMO CSV semantic are sometimes unclear to a lot of people so found better to explain that's the input in the spending transaction. Added missing to.

lightning/src/chain/keysinterface.rs

TheBlueMatt · 2020-05-08T17:05:45Z

lightning/src/chain/keysinterface.rs

 	/// These are generally the result of a "revocable" output to us, spendable only by us unless
 	/// it is an output from us having broadcast an old state (which should never happen).
+	///
+	/// WitnessScript may be regenerated by passing the revocation_pubkey, to_self_delay and


Can you mention somewhere that these are the things that are given by the users' ChannelKeys if they're using a custom ChannelKeys? I dont understand how a user using KeysManager/InMemoryChannelKeys can get this info, though? AFAICT, using those its entirely impossible to spend funds from such an event.

Right, added a commit commenting this far better, and from where you should get each piece of the witnessScript. Tell me if you think it's good enough.

ariard · 2020-05-09T01:55:12Z

As discussed offline, updated with a hallmark for StaticOutputRemotePayment/DynamicOutputP2WSH descriptors.

TheBlueMatt · 2020-05-11T02:07:28Z

lightning/src/chain/keysinterface.rs

@@ -345,6 +421,7 @@ impl ChannelKeys for InMemoryChannelKeys {
 	fn htlc_base_key(&self) -> &SecretKey { &self.htlc_base_key }
 	fn commitment_seed(&self) -> &[u8; 32] { &self.commitment_seed }
 	fn pubkeys<'a>(&'a self) -> &'a ChannelPublicKeys { &self.local_channel_pubkeys }
+	fn hallmark(&self) -> &u32 { &self.hallmark }


If its smaller than 2ish u64s, dont use a reference.

TheBlueMatt · 2020-05-11T02:11:15Z

lightning/src/chain/keysinterface.rs

+		Sha256::from_engine(sha).into_inner()
+	}
+
+	fn derive_channel_keys(&self, channel_value_satoshis: u64, hallmark: u32) -> InMemoryChannelKeys {


If we're going to rely on users being able to regenerate channel keys, we can't use self.unique_start in the generation of them - if you serialize the keys and then read them back from disk they'll be different!

Also, without unique_start here I'm pretty uneasy - in general we try to protect the user from accidentally using the same keys twice on reload even if they never saved the copy after the original use. Thats the point of unique_start - after restarting you'll always get new keys even if you didn't save the old one's channel_child_index counter.

The easy solution to this is to just include unique_start in the "hallmark" - instead of a u32, which doesn't carry much entropy, make it a u64 pair - the first contains the child_index and unique_start_nanos, the second unique_start_secs.

Okay updated while storing seed, staarting_time_nanos, starting_time_secs in KeysManager to return (chan_id | starting_time_nanos, starting_time_secs) as part of ChannelKeys.

These key state is now part of SpendableOutputDescriptor and therefore can be passed to derive_channel_keys, seed is already required to be well stored between KeyManager serialization.

Let me know if you find names compelling and documentation clear enough for user.

lightning/src/chain/keysinterface.rs

arik-so · 2020-05-20T20:27:19Z

lightning/src/chain/keysinterface.rs

@@ -125,15 +151,16 @@ impl Readable for SpendableOutputDescriptor {
 			}),
 			1u8 => Ok(SpendableOutputDescriptor::DynamicOutputP2WSH {


Somewhat unrelated to the diff itself, but as this keeps getting changed, how will we start ensuring backwards compatibility or migration capabilities in the future?

Yes it would be great to have them more standardized, ideally as https://bitcoinops.org/en/topics/output-script-descriptors/

Part of points to address in the future

lightning/src/chain/keysinterface.rs

arik-so · 2020-05-20T20:42:10Z

lightning/src/chain/keysinterface.rs

@@ -394,6 +473,45 @@ impl ChannelKeys for InMemoryChannelKeys {
 		local_commitment_tx.get_htlc_sigs(&self.htlc_base_key, local_csv, secp_ctx)
 	}

+	fn sign_justice_transaction<T: secp256k1::Signing + secp256k1::Verification>(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &Option<HTLCOutputInCommitment>, on_remote_tx_csv: u16, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {


The nesting here, as in the method below, is pretty deep. Could this be changed to either using ? with corresponding From for () implementations, or using consecutive (non-nested) match clauses for the Options that either return the value to a top-level-scope variable or return from the method?

Good idea, took the consecutive (non-nested) match clauses option.

arik-so · 2020-05-20T20:43:20Z

lightning/src/chain/keysinterface.rs

@@ -449,6 +569,8 @@ impl Readable for InMemoryChannelKeys {
 			InMemoryChannelKeys::make_local_keys(&secp_ctx, &funding_key, &revocation_base_key,
 			                                     &payment_key, &delayed_payment_base_key,
 			                                     &htlc_base_key);
+		let user_id_1 = Readable::read(reader)?;


the fact that the derivation params are, in fact, user IDs should probably be documented a bit more widely

In fact they can be user_id but they can whatever else you want, changed names to params_1/params_2

ariard · 2020-05-20T23:38:35Z

@arik-so Thanks for review, addressed most of your comments at 0e4e5f4!

TheBlueMatt

One Real Bug and a few more comments just having looked through keysinterface.rs.

lightning/src/chain/keysinterface.rs

A dynamic-p2wsh-output like `to_local` on local commitment/HTLC txn require a signature from delayed_payment_key to be spend. Instead of sending private key in descriptor, we ask for spender to derive again the corresponding ChannelKeys based on key state, uniquely identifying a channel and encompassing its unique start data. Descriptor modification is done in next commit.

Add sign_delayed_transaction in ChanSigner to be able to spend SpendableOutputDescriptor in test framework.

Add sign_payment_transaction in ChanSigner to be able to spend SpendableOutputDescriptor in test framework Extend ChannelKeys with remote_pubkeys to access remote revocation basepoint for witnessScript construction.

Dry-up remote pubkeys tracking in one struct. This introduce a duplicate of RemoteTxCache, which is going to be removed in next commit when OnchainTxHandler version is removed.

RemoteTxCache was providing all data needed at transaction signature for any remote HTLC transaction or justice transaction. This move was making the API between OnchainTxHandle akward and scope of responsibilites with ChannelMonitor unclear. Instead scope OnchainTxHandler to transaction-finalization, fee-bumping and broadcast only.

on_remote_tx_csv is the CSV delay encumbering remote transactions revokable outputs as required by local. on_local_tx_csv is the CSV delay encumbering local transactions revokable outputs as required by remote. Local/remote is here defined from a code processing viewpoint, process running this code is "local".

We also remove an unecessary build warning.

Instead of blindly signing provided witnessScript, signer must derive channel keys corresponding to the provided per-commitment-point and regenerate templated witnessScript to ensure its syntax correctness.

`to_local` output or remote output on remote commitment transaction needs a channel keys to be spent. As per-channel keys are derived from KeysManager seed and per-channel secrets those must be backed up by any descriptor bookmarking for latter spend. We test that generating a new KeysManager loaded with such backed-up seed/per-channel secrets return the correct keys for spending a `to_local` output.

TheBlueMatt reviewed Apr 29, 2020

View reviewed changes

ariard force-pushed the 2020-04-cache-in-monitor branch 2 times, most recently from 8b8dd01 to c7761cf Compare April 30, 2020 01:25

ariard force-pushed the 2020-04-cache-in-monitor branch from c7761cf to d9ab6f6 Compare April 30, 2020 01:42

TheBlueMatt reviewed Apr 30, 2020

View reviewed changes

ariard force-pushed the 2020-04-cache-in-monitor branch from d9ab6f6 to 9eb0620 Compare May 1, 2020 02:39

ariard added Hard to Review High Review Priority labels May 2, 2020

TheBlueMatt reviewed May 4, 2020

View reviewed changes

ariard force-pushed the 2020-04-cache-in-monitor branch from cf18536 to 59e20d4 Compare May 6, 2020 01:43

TheBlueMatt mentioned this pull request May 6, 2020

Require static_remotekey #539

Merged

ariard force-pushed the 2020-04-cache-in-monitor branch from 59e20d4 to def3da3 Compare May 6, 2020 22:24

TheBlueMatt reviewed May 8, 2020

View reviewed changes

ariard force-pushed the 2020-04-cache-in-monitor branch 2 times, most recently from 71d1faf to 8ad1ff6 Compare May 9, 2020 01:54

TheBlueMatt reviewed May 11, 2020

View reviewed changes

ariard force-pushed the 2020-04-cache-in-monitor branch from 8ad1ff6 to 42f5077 Compare May 12, 2020 02:02

TheBlueMatt reviewed May 12, 2020

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed May 12, 2020

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed May 12, 2020

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

ariard force-pushed the 2020-04-cache-in-monitor branch from 42f5077 to e3f9454 Compare May 12, 2020 21:43

arik-so reviewed May 20, 2020

View reviewed changes

lightning/src/chain/keysinterface.rs Show resolved Hide resolved

arik-so reviewed May 20, 2020

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

arik-so reviewed May 20, 2020

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

arik-so reviewed May 20, 2020

View reviewed changes

lightning/src/chain/keysinterface.rs Show resolved Hide resolved

arik-so reviewed May 20, 2020

View reviewed changes

ariard force-pushed the 2020-04-cache-in-monitor branch from cd3581f to 0e4e5f4 Compare May 20, 2020 23:37

TheBlueMatt reviewed May 27, 2020

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

Antoine Riard and others added 17 commits May 28, 2020 04:21

Remove SecretKey from DynamicOutputP2WSH descriptor

3188ac9

Add sign_delayed_transaction in ChanSigner to be able to spend SpendableOutputDescriptor in test framework.

Remove SecretKey from DynamicOuputP2WPKH descriptor

2c07f8e

Add sign_payment_transaction in ChanSigner to be able to spend SpendableOutputDescriptor in test framework Extend ChannelKeys with remote_pubkeys to access remote revocation basepoint for witnessScript construction.

Duplicate RemoteTxCache in ChannelMonitor

1d7ed17

Dry-up remote pubkeys tracking in one struct. This introduce a duplicate of RemoteTxCache, which is going to be removed in next commit when OnchainTxHandler version is removed.

Drop remote_csv from OnchainTxHandler, cache it in Monitor's cache

56ec6e0

Rename RemoteTxCache as RemoteCommitmentTransaction and document better

3aef447

We also remove an unecessary build warning.

Imp Writer/Reader for RemoteCommitmentTransaction

7de03e1

Dry-up witnessScript in sign_justice_transaction

479e1c8

Instead of blindly signing provided witnessScript, signer must derive channel keys corresponding to the provided per-commitment-point and regenerate templated witnessScript to ensure its syntax correctness.

Dry-up witnessScript in sign_remote_htlc_transaction

06445b6

Instead of blindly signing provided witnessScript, signer must derive channel keys corresponding to the provided per-commitment-point and regenerate templated witnessScript to ensure its syntax correctness.

Make derive_private_key public

4f84b05

Make get_revokable_redeemscript

03fa056

Remove useless build warnings

22daecb

Document better DynamicOutputP2WSH

42b731d

Significantly clarify key derivation and expose methods referenced

d0c5e9c

ariard force-pushed the 2020-04-cache-in-monitor branch from 0e4e5f4 to 81e358c Compare May 28, 2020 08:22

TheBlueMatt merged commit 9dbce1c into lightningdevkit:master May 28, 2020

TheBlueMatt mentioned this pull request May 28, 2020

Drop ChannelKeys Private Key Methods #632

Merged

		@@ -125,15 +151,16 @@ impl Readable for SpendableOutputDescriptor {
		}),
		1u8 => Ok(SpendableOutputDescriptor::DynamicOutputP2WSH {

Move back to ChannelMonitor RemoteTxCache #610

Move back to ChannelMonitor RemoteTxCache #610

Conversation

ariard commented Apr 28, 2020

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 29, 2020

ariard commented Apr 30, 2020

codecov bot commented Apr 30, 2020 • edited Loading

Codecov Report

ariard commented May 1, 2020

TheBlueMatt commented May 2, 2020

ariard commented May 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented May 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented May 20, 2020

TheBlueMatt left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 30, 2020 •

edited

Loading