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

Anderson/remove deposit record #382

Merged
merged 11 commits into from
Sep 8, 2021
Merged

Anderson/remove deposit record #382

merged 11 commits into from
Sep 8, 2021

Conversation

andersonlee725
Copy link
Contributor

@andersonlee725 andersonlee725 commented Sep 6, 2021 •
edited by lastperson
Loading

Description

Replace deposit records with events

  • Remove all deposit records in every handlers.
  • Emit all necessary parameters in deposit event of bridge contract.

Related Issue Or Context

Closes: #254

How Has This Been Tested? Testing details.

Checked if all necessary parameters and meta data responsed from handlers emitted in deposit event in all cases of erc20, erc721 and generic.

  • In case of erc20, checked if an empty data is emitted.
  • In case of erc721, checked if a data returned from erc721 handler is emitted.
  • In case of generic, checked if a data returned from generic handler is emitted.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation

Checklist:

  • I have commented my code, particularly in hard-to-understand areas.
  • I have ensured that all acceptance criteria (or expected behavior) from issue are met
  • I have updated the documentation locally and in chainbridge-docs.
  • I have added tests to cover my changes.
  • I have ensured that all the checks are passing and green, I've signed the CLA bot

@CLAassistant
Copy link

CLAassistant commented Sep 6, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

lastperson
lastperson suggested changes Sep 6, 2021
View reviewed changes
test/handlers/erc20/deposit.js Outdated Show resolved Hide resolved
contracts/handlers/ERC20Handler.sol Show resolved Hide resolved
contracts/handlers/ERC721Handler.sol Show resolved Hide resolved
P1sar
P1sar suggested changes Sep 7, 2021
View reviewed changes
contracts/Bridge.sol Outdated Show resolved Hide resolved
lastperson
lastperson suggested changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@lastperson lastperson left a comment

Choose a reason for hiding this comment

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

A couple minor fixes.

test/handlers/generic/deposit.js Outdated Show resolved Hide resolved
test/handlers/generic/deposit.js Outdated Show resolved Hide resolved
lastperson
lastperson suggested changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@lastperson lastperson left a comment

Choose a reason for hiding this comment

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

Missing resourceID and return descriptions in the deposit functions.

contracts/Bridge.sol Outdated Show resolved Hide resolved
contracts/Bridge.sol Outdated Show resolved Hide resolved
contracts/Bridge.sol Outdated Show resolved Hide resolved
contracts/Bridge.sol Outdated Show resolved Hide resolved
contracts/handlers/ERC721Handler.sol Outdated Show resolved Hide resolved
contracts/handlers/ERC20Handler.sol Outdated Show resolved Hide resolved
contracts/handlers/ERC20Handler.sol Outdated Show resolved Hide resolved
contracts/handlers/ERC20Handler.sol Show resolved Hide resolved
contracts/handlers/ERC20Handler.sol Show resolved Hide resolved
lastperson
lastperson approved these changes Sep 8, 2021
View reviewed changes
contracts/Bridge.sol Outdated Show resolved Hide resolved
@lastperson lastperson changed the base branch from master to develop September 8, 2021 21:47
@lastperson lastperson merged commit 61ec858 into develop Sep 8, 2021
@lastperson lastperson deleted the anderson/remove-depositRecord branch September 8, 2021 21:48
P1sar added a commit that referenced this pull request Oct 27, 2021
@P1sar @andersonlee725 @lastperson
Develop (#413)
cbfaf9c 
* Anderson/remove deposit record (#382)

* remove DepositRecord in every handler,  modify event in Bridge, modify IDepositExecute interface
* modify test files based on removing deposit records
* add handlerResponse to deposit event and modify test files based on the event

* Rename chainID to domainID (#384)

* Add admin set deposit nonce (#385)

* Сombine vote and execute [Breaking] (#389)

* Remove resources from handler constructors (#393)

* Update dependencies and bump version (#394)

Set disableConfirmationListener: true to avoid spamming ganache during tests. trufflesuite/truffle#3522

* Remove fund functions (#412)

Co-authored-by: andersonlee725 <86676141+andersonlee725@users.noreply.github.com>
Co-authored-by: Oleksii Matiiasevych <oleksii@chainsafe.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@P1sar P1sar P1sar requested changes

@lastperson lastperson lastperson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace deposit records with events
4 participants
@andersonlee725 @CLAassistant @lastperson @P1sar