feat: add validation for complete-deposit transactions #545
Conversation
Update the number of arguments that it needs. Update the trait validation function to take in a context object.
|
These sporadic errors seem to be unrelated to the changes in this PR. |
djordon
changed the title
|
There is no bitmap, I'm going to bed. |
| @@ -30,6 +30,22 @@ pub struct SignerInfo { | |||
| pub signer_public_keys: BTreeSet<PublicKey>, | |||
| } | |||
|
|
|||
| /// Generate a set of public keys for a group of signers | |||
| pub fn generate_signer_set<R>(rng: &mut R, num_signers: usize) -> Vec<PublicKey> | |||
There was a problem hiding this comment.
[Suggestion] I might call this something more like generate_signer_set_public_keys since you're only returning the public keys, or even something like this to be able to get it from any Vec<SignerInfo>.
pub trait SignerSetInfoExt {
fn to_public_keys(&self) -> Vec<PublicKey> {
...
}
}
impl SignerSetInfoExt for Vec<SignerInfo> {}There was a problem hiding this comment.
Oops forgot to update. Hmmm, think I'll update this function name in #555 if you don't mind.
| let db_num = DATABASE_NUM.fetch_add(1, Ordering::SeqCst); | ||
| let pg_store = testing::storage::new_test_database(db_num, true).await; | ||
| let mut rng = rand::rngs::StdRng::seed_from_u64(51); | ||
| let signatures_required = 3; |
There was a problem hiding this comment.
[Question] So here we've got 3 (signatures_required), which I guess is just used to get the deposit requests, and the validate() call will use 2 (default in make_complete_deposit()), does this have any bearing on the test? I guess it shouldn't as long as this is higher than the 2 in the ReqContext, but do we need to test that scenario as well? (Not sure where this value will be coming from in test/mainnet or if this is even the area of code which should account for this)
There was a problem hiding this comment.
It affects the test by way of affecting which deposit requests are considered "accepted". I should at the very least note that fact.
There was a problem hiding this comment.
So I noted this better in 2773454. I'm not too happy with it, but it is fairly unimportant for these tests so I didn't change too much.
|
|
||
| // Generate the transaction and corresponding request context. | ||
| let (mut complete_deposit_tx, ctx) = make_complete_deposit(&deposit_req, &sweep_tx, &chain_tip); | ||
| // Different: Okay, let's make sure we get the deployers do not match. |
| let (mut complete_deposit_tx, ctx) = make_complete_deposit(&deposit_req, &sweep_tx, &chain_tip); | ||
| // Different: The amount cannot exceed the amount in the deposit | ||
| // request. | ||
| complete_deposit_tx.amount = deposit_req.amount + 1000; |
There was a problem hiding this comment.
always better to just use 1 in situations like this imo as it proves the logic is sensitive (i.e. in a >= vs. > scenario)
Description
Closes #479.
Changes
AsContractCall::validatefunction to have a different signature.CompleteDepositV1::validatefunction.CompleteDepositV1struct. These new parameters should be used in the contract call once feat: check against known burn hash when completing deposit #493 gets updated and lands.Note that when #543 gets done we'll probably want to add another integration test, but the logic introduced here should be correct and won't need much updating (famous last words).
Testing Information
This PR adds integration tests that checks for each of the failure modes in the function.
Checklist: