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

Integrate elrond-go-p2p #4485

Merged
merged 7 commits into from
Sep 22, 2022
Merged

Conversation

sstanculeanu
Copy link
Collaborator

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

  • p2p package was moved to elrond-go-p2p

Proposed Changes

  • remove the package and use the new repo

Testing procedure

  • standard system test

@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2022

Codecov Report

Base: 73.42% // Head: 73.06% // Decreases project coverage by -0.35% ⚠️

Coverage data is based on head (07ceaa8) compared to base (ceabff3).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##           rc/v1.4.0    #4485      +/-   ##
=============================================
- Coverage      73.42%   73.06%   -0.36%     
=============================================
  Files            655      616      -39     
  Lines          85369    82742    -2627     
=============================================
- Hits           62682    60456    -2226     
+ Misses         17842    17531     -311     
+ Partials        4845     4755      -90     
Impacted Files Coverage Δ
common/configParser.go 26.76% <100.00%> (ø)
...solvers/topicResolverSender/topicResolverSender.go 100.00% <100.00%> (ø)
factory/networkComponents.go 56.55% <100.00%> (ø)
heartbeat/processor/directConnectionsProcessor.go 100.00% <100.00%> (ø)
process/p2p/interceptedDirectConnectionInfo.go 100.00% <100.00%> (ø)
common/pidQueue.go 93.33% <0.00%> (-3.34%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@iulianpascalau iulianpascalau self-requested a review September 20, 2022 10:24
@@ -11,11 +11,10 @@ require (
github.com/ElrondNetwork/elrond-go-core v1.1.19
github.com/ElrondNetwork/elrond-go-crypto v1.0.1
github.com/ElrondNetwork/elrond-go-logger v1.0.7
github.com/ElrondNetwork/elrond-go-p2p v0.0.0-20220919155246-aaae1065ad8b
Copy link
Contributor

Choose a reason for hiding this comment

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

we need a proper release for this

@@ -2513,14 +2513,14 @@ func (tpn *TestProcessorNode) GetShardHeader(nonce uint64) (data.HeaderHandler,

headerObjects, _, err := tpn.DataPool.Headers().GetHeadersByNonceAndShardId(nonce, tpn.ShardCoordinator.SelfId())
if err != nil {
return nil, errors.New(fmt.Sprintf("no headers found for nonce %d and shard id %d %s", nonce, tpn.ShardCoordinator.SelfId(), err.Error()))
return nil, fmt.Errorf("%w no headers found for nonce %d and shard id %d", err, nonce, tpn.ShardCoordinator.SelfId())
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

"github.com/ElrondNetwork/elrond-go-p2p/libp2p"
)

// ListenAddrWithIp4AndTcp defines the listening address with ip v.4 and TCP
Copy link
Contributor

Choose a reason for hiding this comment

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

we can move all these outside the libp2p package to be directly in elrond-go/p2p because the p2p package will no hold only the adapter funcs/structs/consts/errors and so on

// NewMockMessenger creates a new sandbox testable instance of libP2P messenger
// It should not open ports on current machine
// Should be used only in testing!
func NewMockMessenger(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is only used in tests, I think we can remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as discussed, added todo for next PR

func (m *Message) Timestamp() int64 {
return m.TimestampField
}
type Message = message.Message
Copy link
Contributor

Choose a reason for hiding this comment

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

we can move this directly in p2p package

type ArgPeersRatingHandler = rating.ArgPeersRatingHandler

// NewPeersRatingHandler returns a new peers rating handler
func NewPeersRatingHandler(args ArgPeersRatingHandler) (p2p.PeersRatingHandler, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

also this, we can move it directly in p2p

gabi-vuls
gabi-vuls previously approved these changes Sep 21, 2022
Copy link
Contributor

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

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

System test passed.

iulianpascalau
iulianpascalau previously approved these changes Sep 21, 2022
@sstanculeanu sstanculeanu changed the base branch from rc/v1.4.0 to feat/p2p-separate-repo September 21, 2022 14:35
@ssd04 ssd04 self-requested a review September 21, 2022 14:55
Comment on lines +84 to +85
// SyncTimer represent an entity able to tell the current time
type SyncTimer interface {
Copy link
Contributor

@ssd04 ssd04 Sep 21, 2022

Choose a reason for hiding this comment

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

i think we don't need to define this interface here also, it's being used in factory only, so there i think that the interface from ntp package can be used; but can be done later on

@sstanculeanu sstanculeanu merged commit c557cf3 into feat/p2p-separate-repo Sep 22, 2022
@sstanculeanu sstanculeanu deleted the integrate-elrond-go-p2p branch September 22, 2022 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants