Constellation-Labs / tessellation

Monadic execution contexts for topology organization
Apache License 2.0
49 stars 28 forks source link

Add CLI command to merge currency message files #870

Closed sdavidp closed 4 months ago

sdavidp commented 4 months ago

This has been tested with the following test cases:

  1. One valid staking message file
  2. Several identical staking messages files
  3. Several valid staking message files and one with bad json (produces error)
  4. Several valid staking message files and one with bad proof (produces error)
  5. Several valid staking message files and one with different ordinal (produces error)
  6. Several valid staking messages files with different proofs, including one which contains all proofs: result is identical to the file with all the proofs.
sdavidp commented 4 months ago

In general LGTM. I'm ok with it throwing an error and not merging any signatures if there is a single bad apple as long as the user gets a clear feedback on which object/signature was faulty so that discarding (or modifying) file with wrong input is possible (no trial and error needed). We could also try to merge the majority of objects and signatures so the user doesn't have to clean up but we would need to clearly indicate that not all messages were uniform/signed correctly or both. Given that we want to push it forward I think current approach pushing the responsibility to the user is ok.

So I would only verify whether from the error returned a user is able to adjust the command and exclude failing files and I would also consider whether the signatures shouldn't be ordered.

@ryle-goehausen What are your thoughts?

sdavidp commented 4 months ago

In general LGTM. I'm ok with it throwing an error and not merging any signatures if there is a single bad apple as long as the user gets a clear feedback on which object/signature was faulty so that discarding (or modifying) file with wrong input is possible (no trial and error needed). We could also try to merge the majority of objects and signatures so the user doesn't have to clean up but we would need to clearly indicate that not all messages were uniform/signed correctly or both. Given that we want to push it forward I think current approach pushing the responsibility to the user is ok. So I would only verify whether from the error returned a user is able to adjust the command and exclude failing files and I would also consider whether the signatures shouldn't be ordered.

@TheMMaciek I thought of that too and started keeping the files associated with the inputs but decided to first keep it simple in case that wasn't necessary. I'm not sure how we'd report which file is the problem when we fail on mismatched message values, but for invalid signatures it is easier if we mention which file is the culprit. Even without the file I think the user can still figure it out because the id and signature is displayed.

InvalidSignatures{invalidSignatures=NonEmptySortedSet(SignatureProof{id=ba43050d,signature=30460221})}

My examples are contrived so there are multiple files with the same id and signature but I suspect in practice grepping for the offending id/sig prefix will find the bad file.

WDYT?

TheMMaciek commented 4 months ago

@TheMMaciek I thought of that too and started keeping the files associated with the inputs but decided to first keep it simple in case that wasn't necessary. I'm not sure how we'd report which file is the problem when we fail on mismatched message values, but for invalid signatures it is easier if we mention which file is the culprit. Even without the file I think the user can still figure it out because the id and signature is displayed.

InvalidSignatures{invalidSignatures=NonEmptySortedSet(SignatureProof{id=ba43050d,signature=30460221})}

My examples are contrived so there are multiple files with the same id and signature but I suspect in practice grepping for the offending id/sig prefix will find the bad file.

WDYT?

I agree that determining which file is the "correct" one in terms of the mismatched message is hard - we would need to determine majority on the messages which already is a part of the functionality we want to avoid. So maybe just indicating that the message is not uniform across all the files is enough - the user can compare that part with any comparing tool. We could also assume that the first file is the reference file and compare messages against this one - then we could indicate exactly which files are offending. For the signatures I think we could explicitly say which signature for which id and file is wrong (as this is after determining message uniformity).

sdavidp commented 4 months ago

@TheMMaciek I thought of that too and started keeping the files associated with the inputs but decided to first keep it simple in case that wasn't necessary. I'm not sure how we'd report which file is the problem when we fail on mismatched message values, but for invalid signatures it is easier if we mention which file is the culprit. Even without the file I think the user can still figure it out because the id and signature is displayed.

InvalidSignatures{invalidSignatures=NonEmptySortedSet(SignatureProof{id=ba43050d,signature=30460221})}

My examples are contrived so there are multiple files with the same id and signature but I suspect in practice grepping for the offending id/sig prefix will find the bad file.

WDYT?

I agree that determining which file is the "correct" one in terms of the mismatched message is hard - we would need to determine majority on the messages which already is a part of the functionality we want to avoid. So maybe just indicating that the message is not uniform across all the files is enough - the user can compare that part with any comparing tool. We could also assume that the first file is the reference file and compare messages against this one - then we could indicate exactly which files are offending. For the signatures I think we could explicitly say which signature for which id and file is wrong (as this is after determining message uniformity).

Well the message is fairly simple right now -- just 3 fields -- so it should be easy to see which doesn't match.

case class CurrencyMessage(messageType: MessageType, address: Address, parentOrdinal: MessageOrdinal)