Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Escrow account initialization #8247

Merged
merged 11 commits into from
Mar 13, 2023
Merged

Conversation

has5aan
Copy link
Contributor

@has5aan has5aan commented Mar 9, 2023

What was the problem?

This PR resolves #8165

How was it solved?

Added escrow initialisation in sidechain registration command execution.
Added check for receiving chain id in transfer cross chain command.
Added check for receiving chain id in initializeEscrowAccount
Added zero balance check in lock and unlock token module method.

As per the issue details;

Fix payMessageFee method in token module as PR.

payMessageFee method in token module didn't require any changes.

How was it tested?

Implemented unit tests.

@has5aan has5aan requested review from shuse2 and gkoumout March 9, 2023 08:21
@has5aan has5aan self-assigned this Mar 9, 2023
@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #8247 (3fd078d) into development (84fbbfa) will increase coverage by 0.02%.
The diff coverage is 95.83%.

❗ Current head 3fd078d differs from pull request most recent head 73ebe52. Consider uploading reports for the commit 73ebe52 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #8247      +/-   ##
===============================================
+ Coverage        83.06%   83.09%   +0.02%     
===============================================
  Files              589      589              
  Lines            21807    21810       +3     
  Branches          3185     3192       +7     
===============================================
+ Hits             18115    18122       +7     
+ Misses            3692     3688       -4     
Impacted Files Coverage Δ
...k/src/modules/interoperability/mainchain/module.ts 89.79% <0.00%> (ø)
...erability/mainchain/commands/register_sidechain.ts 100.00% <100.00%> (ø)
...src/modules/token/commands/transfer_cross_chain.ts 96.72% <100.00%> (+0.11%) ⬆️
framework/src/modules/token/method.ts 96.72% <100.00%> (+0.21%) ⬆️

... and 8 files with indirect coverage changes

@shuse2 shuse2 changed the title 8165 escrow account initialization Escrow account initialization Mar 9, 2023
Copy link

@gkoumout gkoumout left a comment

Choose a reason for hiding this comment

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

Looks good. However some parts of the LIP PR were overlooked in the corresponding issue, so I added comments on things to be added.

Also, I don't understand why no change was needed in payMessageFee. Was the implementation not aligned with the previous protocol?

@has5aan
Copy link
Contributor Author

has5aan commented Mar 9, 2023

Also, I don't understand why no change was needed in payMessageFee. Was the implementation not aligned with the previous protocol?

To the best of my knowledge the changes recommended by the LIP for payMessageFee are aligned with the current implementation.

@has5aan has5aan requested a review from gkoumout March 9, 2023 18:56
@shuse2 shuse2 enabled auto-merge (squash) March 13, 2023 14:15
@shuse2 shuse2 merged commit c527b6a into development Mar 13, 2023
@shuse2 shuse2 deleted the 8165-escrow-account-initialization branch March 13, 2023 14:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix escrow account initialization
3 participants