Send a legacy PegIn transaction from a Multisig address #2901
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
julianlen
force-pushed
the
pegin-it-differentSenders
branch
3 times, most recently
from
17f7ceb to
62e95ab
Compare
…nder is invalid and throws the right event
julianlen
force-pushed
the
pegin-it-differentSenders
branch
from
62e95ab to
c9394d9
Compare
|
Before reviewing this PR check #2888 |
| Set<PegoutsWaitingForConfirmations.Entry> pegoutEntries = bridgeStorageProvider.getPegoutsWaitingForConfirmations().getEntries(); | ||
| int expectedPegoutsWaitingForConfirmations = 1; | ||
| assertEquals(expectedPegoutsWaitingForConfirmations, pegoutEntries.size()); | ||
|
|
||
| Optional<PegoutsWaitingForConfirmations.Entry> pegOutWaitingForConfirmationOptional = pegoutEntries.stream().findFirst(); | ||
| assertTrue(pegOutWaitingForConfirmationOptional.isPresent()); |
There was a problem hiding this comment.
Maybe we can do the findFirst or size strategy for both of them. For consistency.
There was a problem hiding this comment.
Not sure what you mean, I use both approaches to say: that it's the only one and it's exactly the one I am expecting
There was a problem hiding this comment.
I see, the size assertion indirectly ensures that there is exactly one pegout entry, making the second assertion somewhat redundant. Not a big deal, but you can simplify the code to have a single assert while maintaining clarity.
There was a problem hiding this comment.
Okay, I always wonder whether asserting an isPresent with an optional makes sense. The list of PegoutsWaitingForConfirmations.Entry may have a list of empty optionals
There was a problem hiding this comment.
Since you have checked that it has a size 1 it is guaranteed that the Optional is present.
rskj-core/src/test/java/co/rsk/peg/RegisterBtcTransactionIT.java
Outdated
Show resolved
Hide resolved
rskj-core/src/test/java/co/rsk/peg/RegisterBtcTransactionIT.java
Outdated
Show resolved
Hide resolved
rskj-core/src/test/java/co/rsk/peg/RegisterBtcTransactionIT.java
Outdated
Show resolved
Hide resolved
| private void setupChainWithBtcTransaction(BtcTransaction bitcoinTransaction) throws BlockStoreException { | ||
| pmtWithTransactions = createValidPmtForTransactions(List.of(bitcoinTransaction.getHash()), btcNetworkParams); | ||
| btcBlockWithPmtHeight = bridgeConstants.getBtcHeightWhenPegoutTxIndexActivates() + bridgeConstants.getPegoutTxIndexGracePeriodInBtcBlocks(); | ||
| int chainHeight = btcBlockWithPmtHeight + bridgeConstants.getBtc2RskMinimumAcceptableConfirmations(); | ||
| recreateChainFromPmt(btcBlockStoreWithCache, chainHeight, pmtWithTransactions, btcBlockWithPmtHeight, btcNetworkParams); | ||
| } |
There was a problem hiding this comment.
Does it make sense to have this be changing state class per test? Would it be possible or does it make sense to return assigments to local variables per test?
There was a problem hiding this comment.
You mean having pmtWithTransactions and btcBlockWithPmtHeight as local variables in each test?
There was a problem hiding this comment.
Yes, it seems that it makes more sense to have this at per test level rather than at a per class level.
…typos, improved the name of the test
…l variables in each test
|
No description provided.