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

Avoid direct Transact calls #200

Merged
merged 51 commits into from
Dec 13, 2021
Merged

Avoid direct Transact calls #200

merged 51 commits into from
Dec 13, 2021

Conversation

MakMuftic
Copy link
Contributor

@MakMuftic MakMuftic commented Nov 25, 2021 •
edited
Loading

Description

The idea of this refactor is to enable a few things:

  • Enable multiple implementations of sending transactions - using Transactor interface (this is needed for ITX provider)
  • Easier testing. Now it is possible to mock Transactor interface and also when needed Erc721Contract can be mocked.
  • More readable code - contract calls now accept only parameters relevant to a specific call.

Related Issue Or Context

Closes: #124
Closes: #214

How Has This Been Tested? Testing details.

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

@github-actions
Copy link

Go Test coverage is 32.6 %\ ✨ ✨ ✨

@github-actions
Copy link

Go Test coverage is 32.6 %\ ✨ ✨ ✨

P1sar
P1sar reviewed Nov 26, 2021
View reviewed changes
Copy link
Member

@P1sar P1sar left a comment

Choose a reason for hiding this comment

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

Generally it looks pretty fine, but i guess it will be more interesting to see changes that will happen after Bridge refactor, because current changes affects only CLI basically

@MakMuftic
Fix bridge unit tests and refactor contract calling
27068bf
@MakMuftic
Remove prepare methods from bridge contract
1c821ee
@MakMuftic
Use private function for executing transactions
c6de699
@MakMuftic
Remove test logs
1aa0d12
@MakMuftic
Merge branch 'main' of github.com:ChainSafe/chainbridge-core into mmu…
ae850e2 
…ftic/avoid-direct-transact-calls

# Conflicts:
#	chains/evm/calls/bridge.go
#	chains/evm/calls/deploy.go
#	chains/evm/cli/local/deploy.go
#	chains/evm/evmclient/evm-client.go
#	chains/evm/voter/voter.go
#	e2e/evm-evm/example/app/app.go
#	e2e/evm/test.go
@github-actions
Copy link

github-actions bot commented Dec 1, 2021

Go Test coverage is 39.6 %\ ✨ ✨ ✨

@github-actions
Copy link

github-actions bot commented Dec 1, 2021

Go Test coverage is 39.6 %\ ✨ ✨ ✨

P1sar
P1sar reviewed Dec 2, 2021
View reviewed changes
@github-actions
Copy link

github-actions bot commented Dec 9, 2021

Go Test coverage is 43.4 %\ ✨ ✨ ✨

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

Go Test coverage is 43.4 %\ ✨ ✨ ✨

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

Go Test coverage is 43.4 %\ ✨ ✨ ✨

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

Go Test coverage is 43.6 %\ ✨ ✨ ✨

1 similar comment
@github-actions
Copy link

github-actions bot commented Dec 9, 2021

Go Test coverage is 43.6 %\ ✨ ✨ ✨

@MakMuftic MakMuftic requested a review from tcar121293 December 9, 2021 15:34
P1sar
P1sar suggested changes Dec 10, 2021
View reviewed changes
@github-actions
Copy link

Go Test coverage is 43.5 %\ ✨ ✨ ✨

@github-actions
Copy link

Go Test coverage is 43.5 %\ ✨ ✨ ✨

@MakMuftic MakMuftic requested a review from P1sar December 13, 2021 12:08
@github-actions
Copy link

Go Test coverage is 42.8 %\ ✨ ✨ ✨

@github-actions
Copy link

Go Test coverage is 42.8 %\ ✨ ✨ ✨

@mpetrun5 mpetrun5 merged commit 28e79b8 into main Dec 13, 2021
@mpetrun5 mpetrun5 deleted the mmuftic/avoid-direct-transact-calls branch December 13, 2021 15:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mpetrun5 mpetrun5 mpetrun5 approved these changes

@P1sar P1sar P1sar approved these changes

@tcar121293 tcar121293 tcar121293 approved these changes

@nmlinaric nmlinaric nmlinaric approved these changes

@waymobetta waymobetta Awaiting requested review from waymobetta

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

Successfully merging this pull request may close these issues.

Fix e2e tests Avoid direct Transact calls
5 participants
@MakMuftic @P1sar @tcar121293 @mpetrun5 @nmlinaric