Skip to content

Conversation

@mlal20
Copy link

@mlal20 mlal20 commented Oct 23, 2024

We have taken refreance from baby swap adopter , please review and let us know if anythings need to change.

operations.pop();
// if it is not empty, we fill the other left operations.
if !operations.is_empty() {
hops.extend(operations.into_iter().map(|op| Hop {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this make the hops vector have the first hop twice? Since the first_hop is added at initialization of hops and then his extended here ?

Copy link
Author

Choose a reason for hiding this comment

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

@NotJeremyLiu I also had some confusions releted to this so could you help me understand and what would be the better approach or current one is fine.

Copy link
Member

Choose a reason for hiding this comment

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

not sure, I imagine it's not including the first operation when you initialize the vector. You can write tests to verify it works as you want it to.

Copy link
Author

Choose a reason for hiding this comment

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

@NotJeremyLiu we have checked we skipped the first operations then it is not working as expected and the hops vector does not have first hop twice so please check and let us know if any things else is required as we have executed test cases and they are working fine

Copy link
Member

@NotJeremyLiu NotJeremyLiu left a comment

Choose a reason for hiding this comment

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

left some comments, great work, once comments are responded to/address please ask for a re-review

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.

2 participants