Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds 'should create multiple pegouts with mixed values same and above… #163

Open
wants to merge 2 commits into
base: rits-refactors-9-2024-integration
Choose a base branch
from

Conversation

jeremy-then
Copy link
Contributor

Adds 'should create multiple pegouts with mixed values same and above minimum' test

@jeremy-then jeremy-then self-assigned this Oct 31, 2024
@jeremy-then jeremy-then requested a review from a team as a code owner October 31, 2024 01:29
Copy link

@apancorb apancorb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor comments

const txPromise = rskTxHelper.sendTransaction({
from: rskFromAddress,
to: BRIDGE_ADDRESS,
value: amountInWeisBN,
gasPrice: TO_BRIDGE_GAS_PRICE,
});

if(!mine) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
if(!mine) {
if (!mine) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed there is a mixed of if ( an if( in the test. I always prefer if(. But what should we always use? @nathanieliov , @marcos-iov , @julia-zack .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apancorb 's suggestion, as it seems to be the rule of our formatting settings. I formatted it in my local and the result was @apancorb suggestion.

lib/rsk-utils.js Show resolved Hide resolved

await triggerRelease(rskTxHelpers, btcTxHelper, callbacks);

// Checking all the pegout events are emitted and in order

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems they all get mined in the same block. How are we verifying the order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They all get mined in the same block, otherwise, the test would fail.
Since they can get mined in different order, I'm forced to use pegoutsEvents.find index of indices and getAddressUtxo to find the corresponding utxos.

lib/tests/2wp.js Show resolved Hide resolved
const txPromise = rskTxHelper.sendTransaction({
from: rskFromAddress,
to: BRIDGE_ADDRESS,
value: amountInWeisBN,
gasPrice: TO_BRIDGE_GAS_PRICE,
});

if(!mine) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apancorb 's suggestion, as it seems to be the rule of our formatting settings. I formatted it in my local and the result was @apancorb suggestion.

lib/rsk-utils.js Show resolved Hide resolved
Base automatically changed from add-pegout-with-weis-rounded-down-to-satoshis-test to add-pegout-request-from-contract-test November 6, 2024 02:08
@jeremy-then jeremy-then force-pushed the add-multiple-pegouts-batch-test branch from 2ac0319 to 5c567a3 Compare November 6, 2024 02:21
@marcos-iov marcos-iov force-pushed the add-pegout-request-from-contract-test branch from 9c1d2e2 to 253411e Compare November 6, 2024 11:39
Base automatically changed from add-pegout-request-from-contract-test to rits-refactors-9-2024-integration November 6, 2024 11:47
@jeremy-then jeremy-then force-pushed the add-multiple-pegouts-batch-test branch from 5c567a3 to bc9cb09 Compare November 7, 2024 19:13
Copy link

sonarcloud bot commented Nov 7, 2024

Comment on lines +968 to +970
const pegoutCreatedCallback = async () => {
bridgeStateAfterPegoutCreation = await getBridgeState(rskTxHelper.getClient());
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, what's the value of this callback? It will simply get the Bridge state and do nothing with it

const pegoutTransaction2 = await pegoutTransaction2Promise;
const pegoutTransaction3 = await pegoutTransaction3Promise;

// Assert
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check the Bridge state at this point? To assert there are 3 pegout requests created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants