Adds 'should reject and refund a pegout when fee per kb is above valu… #140
Conversation
lib/tests/2wp.js
Outdated
|
|
||
| // Create a pegin for the sender to ensure there is enough funds to pegout and because this is the natural process | ||
| const senderRecipientInfo = await createSenderRecipientInfo(rskTxHelper, btcTxHelper); | ||
| const peginValueInSatoshis = btcToSatoshis(0.5); |
There was a problem hiding this comment.
Maybe we can have a default constant for 0.5 as well?
There was a problem hiding this comment.
I think there's no need for this. This is not the minimum pegin value or any significan value. This is just to have enough funds on the rsk address. This value could change from test to test. For example, in the future pegout test where we will need to create a lot of pegouts to assert that the Bridge split them, we will probably need to send way more funds. So, that value could change.
There was a problem hiding this comment.
As long as this value does not repeat in other places, it is ok to me.
There was a problem hiding this comment.
Left a similar comment in another PR.
Not a big deal, open to discuss it. If the value won't always be the same then maybe it doesn't make sense to have a constant. But it could maybe at least be a constant within the test to not make it a magic number.
Option 2 is to calculate this value depending on what the test needs. If we are doing a peg-out for pegoutValue=X, then this value could be pegoutValue * 2
Open to hear other ideas
jeremy-then
force-pushed
the
add-pegout-with-fee-above-value-test
branch
from
57c7280 to
08d4d77
Compare
jeremy-then
force-pushed
the
add-should-reject-and-refund-pegout-below-minimum-test
branch
from
36834c7 to
c792c63
Compare
jeremy-then
force-pushed
the
add-pegout-with-fee-above-value-test
branch
from
08d4d77 to
aed2792
Compare
lib/constants.js
Outdated
| @@ -116,6 +116,9 @@ const PEGIN_V1_RSKT_PREFIX_HEX = '52534b54'; | |||
|
|
|||
| const MINIMUM_PEGOUT_AMOUNT_IN_RBTC = 0.0025; | |||
|
|
|||
| const FEE_PER_KB_CHANGE_PK = '6a4b49312b91e203ddfb9bc2d900ebbd46fbede46a7462e770bedcb11ad405e9'; | |||
There was a problem hiding this comment.
A bit nit from my side, but whenever I see PK I wish I could tell if it means public or private key. And the difference is quite important
| await sendTransaction(rskTxHelper, bridge.methods.voteFeePerKbChange(feePerKbInSatoshis), FEE_PER_KB_CHANGE_ADDR); | ||
|
|
||
| const finalFeePerKb = await bridge.methods.getFeePerKb().call(); | ||
| expect(Number(finalFeePerKb)).to.equal(Number(feePerKbInSatoshis)); |
There was a problem hiding this comment.
You could also do a call before sending the transaction, and assert the response code
There was a problem hiding this comment.
A call to voteFeePerKbChange or getFeePerKb?
There was a problem hiding this comment.
Updated, by calling sendTxWithCheck.
lib/tests/2wp.js
Outdated
|
|
||
| // Create a pegin for the sender to ensure there is enough funds to pegout and because this is the natural process | ||
| const senderRecipientInfo = await createSenderRecipientInfo(rskTxHelper, btcTxHelper); | ||
| const peginValueInSatoshis = btcToSatoshis(0.5); |
There was a problem hiding this comment.
Left a similar comment in another PR.
Not a big deal, open to discuss it. If the value won't always be the same then maybe it doesn't make sense to have a constant. But it could maybe at least be a constant within the test to not make it a magic number.
Option 2 is to calculate this value depending on what the test needs. If we are doing a peg-out for pegoutValue=X, then this value could be pegoutValue * 2
Open to hear other ideas
lib/tests/2wp.js
Outdated
| const btcPeginTxHash = await sendPegin(rskTxHelper, btcTxHelper, senderRecipientInfo.btcSenderAddressInfo, satoshisToBtc(peginValueInSatoshis)); | ||
| await ensurePeginIsRegistered(rskTxHelper, btcPeginTxHash); |
There was a problem hiding this comment.
Could this whole part maybe be moved to a method? fundBridgeForPegout or something of the sort. What do you think?
jeremy-then
force-pushed
the
add-should-reject-and-refund-pegout-below-minimum-test
branch
from
c792c63 to
c8d5e3e
Compare
jeremy-then
force-pushed
the
add-pegout-with-fee-above-value-test
branch
from
58d00aa to
102700b
Compare
jeremy-then
force-pushed
the
add-pegout-with-fee-above-value-test
branch
from
f39a419 to
f5065f6
Compare
lib/tests/2wp.js
Outdated
| const initial2wpBalances = await get2wpBalances(rskTxHelper, btcTxHelper); | ||
| const initialBtcRecipientAddressBalanceInSatoshis = await getBtcAddressBalanceInSatoshis(btcTxHelper, senderRecipientInfo.btcSenderAddressInfo.address); | ||
| const initialRskSenderBalanceInWeisBN = await rskTxHelper.getBalance(senderRecipientInfo.rskRecipientRskAddressInfo.address); | ||
| const pegoutValueInSatoshis = MINIMUM_PEGOUT_AMOUNT_IN_SATOSHIS - 1; |
There was a problem hiding this comment.
Is this a good idea? We want the pegout to be rejected for the fee value, I wouldn't see an invalid pegout request. That could create some confusion
|
marcos-iov
merged commit 1aecc22
into
rits-refactors-9-2024-integration
Adds 'should reject and refund a pegout when fee per kb is above value' test