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.

Sign up for GitHub

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

Separate chain interface for the coordinator package #3650

Merged
merged 11 commits into from
Jun 27, 2023
Merged

Separate chain interface for the coordinator package #3650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1343277
Extract chain interface for sweep proposal validation
nkuba Jun 26, 2023
01b0dae
Define separate chain interface for pkg/coordinator
nkuba Jun 26, 2023
9f21e9f
Reflect latest chain interfaces refactoring in test implementations
nkuba Jun 26, 2023
d4cfb52
Extract chain interface for redemption proposal validation
nkuba Jun 26, 2023
0760625
Update local chain implementation for wallet maintainer
nkuba Jun 26, 2023
70908bd
Add TODO question for PastDepositSweepProposalSubmittedEvents
nkuba Jun 26, 2023
d55dfa2
Clean imports
nkuba Jun 26, 2023
74e0933
Update local chain implementation for coordinator package
nkuba Jun 26, 2023
7eeea1f
Rename local chain file for consistency
nkuba Jun 26, 2023
70bd449
Inline chain interface for validation
nkuba Jun 27, 2023
da06660
Remove PastDepositSweepProposalSubmittedEvents function
nkuba Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 8 additions & 66 deletions pkg/chain/ethereum/tbtc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@ import (
"sort"
"time"

"github.com/keep-network/keep-common/pkg/chain/ethereum/ethutil"
"github.com/keep-network/keep-core/pkg/bitcoin"
"github.com/keep-network/keep-core/pkg/coordinator"

"github.com/ethereum/go-ethereum/accounts/abi"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/crypto"
"github.com/keep-network/keep-common/pkg/chain/ethereum/ethutil"
"github.com/keep-network/keep-core/pkg/bitcoin"

"github.com/keep-network/keep-common/pkg/chain/ethereum"
"github.com/keep-network/keep-core/pkg/chain"
Expand Down Expand Up @@ -1125,8 +1127,8 @@ func (tc *TbtcChain) GetDepositRequest(
}

func (tc *TbtcChain) PastNewWalletRegisteredEvents(
filter *tbtc.NewWalletRegisteredEventFilter,
) ([]*tbtc.NewWalletRegisteredEvent, error) {
filter *coordinator.NewWalletRegisteredEventFilter,
) ([]*coordinator.NewWalletRegisteredEvent, error) {
var startBlock uint64
var endBlock *uint64
var ecdsaWalletID [][32]byte
Expand All @@ -1149,9 +1151,9 @@ func (tc *TbtcChain) PastNewWalletRegisteredEvents(
return nil, err
}

convertedEvents := make([]*tbtc.NewWalletRegisteredEvent, 0)
convertedEvents := make([]*coordinator.NewWalletRegisteredEvent, 0)
for _, event := range events {
convertedEvent := &tbtc.NewWalletRegisteredEvent{
convertedEvent := &coordinator.NewWalletRegisteredEvent{
EcdsaWalletID: event.EcdsaWalletID,
WalletPublicKeyHash: event.WalletPubKeyHash,
BlockNumber: event.Raw.BlockNumber,
Expand Down Expand Up @@ -1453,66 +1455,6 @@ func (tc *TbtcChain) OnDepositSweepProposalSubmitted(
OnEvent(onEvent)
}

func (tc *TbtcChain) PastDepositSweepProposalSubmittedEvents(
filter *tbtc.DepositSweepProposalSubmittedEventFilter,
) ([]*tbtc.DepositSweepProposalSubmittedEvent, error) {
var startBlock uint64
var endBlock *uint64
var coordinator []common.Address
var walletPublicKeyHash [20]byte

if filter != nil {
startBlock = filter.StartBlock
endBlock = filter.EndBlock

for _, ps := range filter.Coordinator {
coordinator = append(
coordinator,
common.HexToAddress(ps.String()),
)
}

walletPublicKeyHash = filter.WalletPublicKeyHash
}

events, err := tc.walletCoordinator.PastDepositSweepProposalSubmittedEvents(
startBlock,
endBlock,
coordinator,
)
if err != nil {
return nil, err
}

convertedEvents := make([]*tbtc.DepositSweepProposalSubmittedEvent, 0)
for _, event := range events {
// If the wallet PKH filter is set, omit all events that target
// different wallets.
if walletPublicKeyHash != [20]byte{} {
if event.Proposal.WalletPubKeyHash != walletPublicKeyHash {
continue
}
}

convertedEvent := &tbtc.DepositSweepProposalSubmittedEvent{
Proposal: convertDepositSweepProposalFromAbiType(event.Proposal),
Coordinator: chain.Address(event.Coordinator.Hex()),
BlockNumber: event.Raw.BlockNumber,
}

convertedEvents = append(convertedEvents, convertedEvent)
}

sort.SliceStable(
convertedEvents,
func(i, j int) bool {
return convertedEvents[i].BlockNumber < convertedEvents[j].BlockNumber
},
)

return convertedEvents, err
}

func convertDepositSweepProposalFromAbiType(
proposal tbtcabi.WalletCoordinatorDepositSweepProposal,
) *tbtc.DepositSweepProposal {
Expand Down
93 changes: 89 additions & 4 deletions pkg/coordinator/chain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,69 @@
package coordinator

import (
"math/big"

"github.com/keep-network/keep-core/pkg/bitcoin"
"github.com/keep-network/keep-core/pkg/tbtc"
"math/big"
)

// NewWalletRegisteredEvent represents a new wallet registered event.
type NewWalletRegisteredEvent struct {
EcdsaWalletID [32]byte
WalletPublicKeyHash [20]byte
BlockNumber uint64
}

// NewWalletRegisteredEventFilter is a component allowing to filter NewWalletRegisteredEvent.
type NewWalletRegisteredEventFilter struct {
StartBlock uint64
EndBlock *uint64
EcdsaWalletID [][32]byte
WalletPublicKeyHash [][20]byte
}

// Chain represents the interface that the coordinator module expects to interact
// with the anchoring blockchain on.
type Chain interface {
// TODO: Change to something more specific once https://github.com/keep-network/keep-core/issues/3632
// is handled.
tbtc.Chain
// GetDepositRequest gets the on-chain deposit request for the given
// funding transaction hash and output index. Returns an error if the
// deposit was not found.
GetDepositRequest(
fundingTxHash bitcoin.Hash,
fundingOutputIndex uint32,
) (*tbtc.DepositChainRequest, error)
Copy link
Member Author

Choose a reason for hiding this comment

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

As for the functions, it's clear that we want to define them separately for each package, even if it means duplication. But what about the types the functions are expecting or returning?
I don't think duplication is the right way. Alternatively to the current solution where we just import the types from the tbtc package, we could alias them, but it doesn't make much sense.

I'm curious about your opinion @pdyraga @lukasz-zimnoch.

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, we should look at the packages as some bounded contexts of the same domain:

  • pkg/tbtc is the context corresponding to the tbtc wallet
  • pkg/coordinator is the context of a tbtc wallet coordinator

That said, both should define domain objects (i.e. types) and functions specific to their contexts. However, some of the domain may be shared between those two contexts and this is the case we observe here. I think we have two options:

  1. Declare the shared objects in one of the contexts
  2. Extract the shared objects to a separate place

I think option 2 is worse in our case as it will introduce unnecessary confusion. In my opinion, we should aim for option 1 as it is more "natural" for our codebase.

The only question is: "Which package should hold the shared objects?". In our case, the pkg/tbtc is kind of a core package while the pkg/coordinator is kind of an overlay specialized for the coordination part. If we agree about that, this implies that all shared objects should live in the pkg/tbtc package.

Copy link
Member

@pdyraga pdyraga Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would be fine with the duplication for now but I think the problem with the duplication is that those structs are separate types and the chain implementation struct would have to redeclare the same function multiple times. For example:

func GetDepositRequest(...) coordinator.DepositChainRequest // pkg/coordinator chain interface
func GetDepositRequest(...) spv.DepositChainRequest // pkg/maintainer/spv chain interface
func GetDepositRequest(...) wallet.DepositChainRequest // pkg/maintainer/wallet chain interface

The current solution is the lesser evil but I think we can do better. Instead of creating a dependency to tbtc package, we could have a separate package with the common type declarations.

pkg/chain/types/types.go

pkg/tbtc/chain.go                           // imports pkg/chain/types
pkg/coordinator/chain.go                    // imports pkg/chain/types
pkg/maintainer/wallet/chain.go              // imports pkg/chain/types
pkg/maintainer/spv/chain.go                 // imports pkg/chain/types

EDIT: Alternatively /pkg/chain/tbtc/types/types.go

Copy link
Member

@lukasz-zimnoch lukasz-zimnoch Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

As an alternative, we could also use a similar pattern that we use in pkg/tecdsa, i.e.:

pkg
    tbtc
        wallet        // holds the current contents of pkg/tbtc
            chain.go  // interfaces and objects specific to wallet context
        coordinator   // holds the current contents of pkg/coordinator
            chain.go  // interfaces and objects specific to coordinator context
        maintainer    // holds the current contents of pkg/maintainer
        chain.go      // interfaces and objects shared by the above sub-packages

Worth noting that all sub-packages can depend on pkg/tbtc. This would correspond to option 1 from my previous comment where we extract the shared domain into a separate place.

Copy link
Member

Choose a reason for hiding this comment

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

I think we intentionally decided to flatten packages compared to pkg/tecdsa and treat pkg/tbtc as the client-side wallet implementation. I don't think the alternative is bad but I also like the fact the packages are currently more flat compared to pkg/tecdsa.

Copy link
Member

Choose a reason for hiding this comment

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

If pkg/tbtc is about the wallet signing logic, the auxiliary maintainer logic shouldn't import it. pkg/tbtc/types is a completely separate package from the Go perspective.

Agreed but in fact, pkg/tbtc is a little bit broader than we assume here. It also contains some logic that is useful for auxiliary packages, for example tbtc.ValidateDepositSweepProposal. That means that even if we extract types to pkg/tbtc/types, we would still have the dependency due to some shared logic. This is why I'm leaning towards leaving shared types in pkg/tbtc and do not complicate the situation with an additional package.

Copy link
Member

Choose a reason for hiding this comment

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

In order to elaborate more on why I'm a bit against pkg/tbtc/types:

  • It's a bit too generic and does not give a clear answer about what should live inside and what not
  • It is nested within pkg/tbtc which will make a wrong dependency direction: pkg/tbtc -> pkg/tbtc/types. In Go, we typically make sub-packages depending on the outer packages (a lot of examples in Go std lib but also ours pkg/net, pkg/tecdsa, and so on).

If we really insist on extracting, I would rather go with creating pkg/tbtccore and put there all core types and shared logic that does not belong to any specific domain context.

Copy link
Member

Choose a reason for hiding this comment

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

  • It is nested within pkg/tbtc which will make a wrong dependency direction: pkg/tbtc -> pkg/tbtc/types. In Go, we typically make sub-packages depending on the outer packages (a lot of examples in Go std lib but also ours pkg/net, pkg/tecdsa, and so on).

Yes, we would have to bend this rule. It is not ideal but we wouldn't be the only ones. For example, the Block structure is defined in github.com/ethereum/go-ethereum/core/types package and it is imported by go-ethereum/core/blockchain_insert.go here. This is exactly our use case. As long as we do it only for the types package, I would be personally fine with it.

  • It's a bit too generic and does not give a clear answer about what should live inside and what not

It is less generic than tbtccore :) I see the reasoning behind the tbtccore but I am afraid this will sooner or later evolve into a common bag of stuff and create even more complicates mesh of dependencies. I think it's nothing wrong if coordinator or maintainer packages imports logic from tbtc. It is clear, they perform the same validation as the wallet as in the mentioned tbtc.ValidateDepositSweepProposal example and in my opinion, this is a really nice feature of the current code organization.

My slight preference is for a separate types package with good documentation of why it's there and what types should be there. This would give us more flexibility and avoid import cycles. But I am also fine with leaving it as-is with imports from tbtc.

Copy link
Member Author

@nkuba nkuba Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, it seems that there is no perfect solution.

If we decide to declare the types in a common types package what types should go there? All the types used by chain interfaces across all the packages? We have types that are common to pkg/tbtc and pkg/coordinator and specific to the pkg/tbtc only. Which do we want to move to the common types?

Separating types from the chain interfaces declaration may be confusing, as in V2 we decided to declare the chain interfaces closer to the actual usage in packages.

Importing types defined in pkg/tbtc package into the pkg/coordinator doesn't seem perfect either. The pkg/coordinator package uses pkg/tbtc types, but also declares its' own types (e.g. NewWalletRegisteredEvent).

We cannot duplicate the types either, as the functions defined in different packages will require the same type, e.g. ValidateRedemptionProposal defined in both pkg/coordinator and pkg/tbtc must use the same RedemptionProposal type.

I lean towards the current approach where types are defined in pkg/tbtc which is considered a broader and core package and we just import them in pkg/coordinator. This isn't perfect but it doesn't require an aggressive refactor.

We could add another issue for the future to find the perfect solution for types definition.

Copy link
Member

Choose a reason for hiding this comment

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

Captured it here: #3652


// PastNewWalletRegisteredEvents fetches past new wallet registered events
// according to the provided filter or unfiltered if the filter is nil. Returned
// events are sorted by the block number in the ascending order, i.e. the
// latest event is at the end of the slice.
PastNewWalletRegisteredEvents(
filter *NewWalletRegisteredEventFilter,
) ([]*NewWalletRegisteredEvent, error)

// BuildDepositKey calculates a deposit key for the given funding transaction
// which is a unique identifier for a deposit on-chain.
BuildDepositKey(fundingTxHash bitcoin.Hash, fundingOutputIndex uint32) *big.Int

// GetDepositParameters gets the current value of parameters relevant
// for the depositing process.
GetDepositParameters() (
dustThreshold uint64,
treasuryFeeDivisor uint64,
txMaxFee uint64,
revealAheadPeriod uint32,
err error,
)

// SubmitDepositSweepProposalWithReimbursement submits a deposit sweep
// proposal to the chain. It reimburses the gas cost to the caller.
SubmitDepositSweepProposalWithReimbursement(
proposal *tbtc.DepositSweepProposal,
) error

// GetDepositSweepMaxSize gets the maximum number of deposits that can
// be part of a deposit sweep proposal.
GetDepositSweepMaxSize() (uint16, error)

// PastRedemptionRequestedEvents fetches past redemption requested events according
// to the provided filter or unfiltered if the filter is nil. Returned
Expand Down Expand Up @@ -56,4 +108,37 @@ type Chain interface {
// the redemption request creation before a request becomes eligible for
// a processing.
GetRedemptionRequestMinAge() (uint32, error)

// PastDepositRevealedEvents fetches past deposit reveal events according
// to the provided filter or unfiltered if the filter is nil. Returned
// events are sorted by the block number in the ascending order, i.e. the
// latest event is at the end of the slice.
PastDepositRevealedEvents(
filter *tbtc.DepositRevealedEventFilter,
) ([]*tbtc.DepositRevealedEvent, error)

// GetPendingRedemptionRequest gets the on-chain pending redemption request
// for the given wallet public key hash and redeemer output script.
// Returns an error if the request was not found.
GetPendingRedemptionRequest(
walletPublicKeyHash [20]byte,
redeemerOutputScript bitcoin.Script,
) (*tbtc.RedemptionRequest, error)

// ValidateDepositSweepProposal validates the given deposit sweep proposal
// against the chain. It requires some additional data about the deposits
// that must be fetched externally. Returns an error if the proposal is
// not valid or nil otherwise.
ValidateDepositSweepProposal(
proposal *tbtc.DepositSweepProposal,
depositsExtraInfo []struct {
*tbtc.Deposit
FundingTx *bitcoin.Transaction
},
) error

// ValidateRedemptionProposal validates the given redemption proposal
// against the chain. Returns an error if the proposal is not valid or
// nil otherwise.
ValidateRedemptionProposal(proposal *tbtc.RedemptionProposal) error
}
Loading