added a test for the getEstimatedFeesForNextPegoutEvent#385
added a test for the getEstimatedFeesForNextPegoutEvent#385
Conversation
julianlen
force-pushed
the
getEstimatedFees-tests
branch
2 times, most recently
from
4dc87a1 to
d1782e7
Compare
julianlen
force-pushed
the
getEstimatedFees-tests
branch
from
d1782e7 to
30c16f7
Compare
julianlen
force-pushed
the
getEstimatedFees-tests
branch
from
41e9b56 to
5356a97
Compare
…rs, call the last one and use it
…dAssertQueueCleared
…egoutRequestToTheBridge
There was a problem hiding this comment.
missing a bunch of test cases: 0 requests, multiple requests, different pegout amounts.
There was a problem hiding this comment.
this wasn't the idea, it is an integration test
There was a problem hiding this comment.
added a second part of each test as discussed checking getEstimatedFees with no pegout requests
julianlen
force-pushed
the
getEstimatedFees-tests
branch
from
9a8f182 to
e937fc3
Compare
julianlen
force-pushed
the
getEstimatedFees-tests
branch
from
bedb34f to
6ea1829
Compare
| @@ -0,0 +1,3 @@ | |||
| const getEstimatedFeesMethodsTests = require('../lib/tests/get_estimated_fees_methods'); | |||
There was a problem hiding this comment.
By the name of the file it will run after activateLatestFork, probably not a good idea.
It would be better to have this along with all the other "normal"
test scenarios. Tests that go after the activation of the latest fork are for testing the upcoming yet on development network upgrade
| let btcTxHelper; | ||
| let senderInfo; | ||
|
|
||
| const shouldReturnGreaterThanZeroErrorMessage = (methodName) => |
There was a problem hiding this comment.
Looks like this is only used with the same param always getEstimatedFeesForNextPegOutEvent so it probably doesn't make sense to keep it as method, could simply be a constant
There was a problem hiding this comment.
done 7855484 (it was used with both methods but for some reason I forgot to changed it to getEstimatedFeesForPegOutAmount)
| ); | ||
| expect(peginUtxoInBridgeState).to.not.be.undefined; | ||
|
|
||
| await sendFromCow( |
There was a problem hiding this comment.
Do we need to send funds from the cow address? Weren't those funds already received from the peg-in?
| Number(btcToWeis(PEGOUT_SENDER_FUNDING_IN_BTC)) | ||
| ); | ||
|
|
||
| await sendTxToBridge( |
There was a problem hiding this comment.
Would it make sense to get the bridge state after this and assert there is a pegout request?
| ); | ||
| }; | ||
|
|
||
| const sendPegoutWithEstimatedFees = async (estimatedFees) => { |
There was a problem hiding this comment.
| const sendPegoutWithEstimatedFees = async (estimatedFees) => { | |
| const sendPegoutWithEstimatedFees = async (estimatedFeesInSatoshis) => { |
Adding the unit helps, it can get confusing vey fast
| // Ensuring there's no pegout pending in the Bridge contract | ||
| // after each test to avoid interference between tests | ||
| afterEach(async () => { | ||
| await triggerRelease(rskTxHelpers, btcTxHelper); |
There was a problem hiding this comment.
Get the bridge state and assert there are no pending pegout requests after this?
| }); | ||
|
|
||
| describe('getEstimatedFeesForNextPegOutEvent', () => { | ||
| it('should build pegouts from getEstimatedFeesForNextPegOutEvent when one pegout is queued and when none are queued, and process all pegouts', async () => { |
There was a problem hiding this comment.
Not sure I follow this test.
get estimated fees with 1 request and assert it's > 0. Then add a second request and immediately process the pegouts, why?
Then get estimated fees with 0 requests and assert it's > 0.
Finally send a new pegout request and immediately process it.
It all seems a bit confusing
| }); | ||
|
|
||
| describe('getEstimatedFeesForPegOutAmount', () => { | ||
| it('should build pegouts from getEstimatedFeesForPegOutAmount when one pegout is queued and when none are queued, and process all pegouts', async () => { |
There was a problem hiding this comment.
Same here.
Also, we should test passsing different values as the param of getEstimatedFeesForPegOutAmount. Perhaps we should have 2 utxos in the Bridge? So for one param it only needs 1 utxo and returns a certain value, for another param it needs 2 utxos to build the transaction so the fees are higher
…esForPegOutAmount
|
4 participants
No description provided.