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

MaintainerProxy bindings #3630

Merged
merged 3 commits into from
Jun 16, 2023
Merged

MaintainerProxy bindings #3630

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9dc0b77
Generated bindings for MaintainerProxy
tomaszslabon Jun 13, 2023
36c0c90
Generated bindings for mainnet
tomaszslabon Jun 15, 2023
800fdda
Added link to issue
tomaszslabon Jun 16, 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
1 change: 1 addition & 0 deletions pkg/chain/ethereum/common/gen/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ endif
contract/%.go cmd/%.go: abi/%.abi abi/%.go _address/% ${artifacts_dir}/%.json
$(info $* - generating Keep bindings)
@go run github.com/keep-network/keep-common/tools/generators/ethereum $< contract/$*.go cmd/$*.go
$(call after_contract_hook,$*)

# Don't remove intermediate files that got generated.
.PRECIOUS: abi/%.abi abi/%.go _address/%
28 changes: 24 additions & 4 deletions pkg/chain/ethereum/tbtc/gen/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
npm_package_name=@keep-network/tbtc-v2

# Contracts for which the bindings should be generated.
required_contracts := Bridge LightRelay LightRelayMaintainerProxy WalletCoordinator
required_contracts := Bridge MaintainerProxy LightRelay LightRelayMaintainerProxy WalletCoordinator

# There is a bug in the currently used abigen version (v1.10.19) that makes it
# re-declaring structs used by multiple contracts
# https://github.com/ethereum/go-ethereum/issues/24627. This is a problem
# for us because both Bridge and WalletCoordinator contracts use BitcoinTx.Info
# struct which is then re-declared in the same package once abigen does its work.
# for us because Bridge, WalletCoordinator and MaintainerProxy contracts all use
# the same structs which are then re-declared in the same package once abigen
# does its work.
# An ultimate solution would be upgrading go-ethereum (thus abigen too) to v1.11 but
# that version contains some breaking changes that make the upgrade non-trivial.
# As a short-term workaround, we use some Makefile shenanigans to slightly rename
# the conflicting BitcoinTxInfo struct in the WalletCoordinator output file.
# the conflicting structs in the WalletCoordinator and MaintainerProxy output files.
# We use perl for that purpose as sed is not cross-platform and works a bit
# differently on GNU and BSD.
#
Expand All @@ -20,9 +21,28 @@ required_contracts := Bridge LightRelay LightRelayMaintainerProxy WalletCoordina
define after_abi_hook
$(eval type := $(1))
$(if $(filter $(type),WalletCoordinator),$(call fix_wallet_coordinator_collision))
$(if $(filter $(type),MaintainerProxy),$(call fix_maintainer_proxy_collision))
endef
define fix_wallet_coordinator_collision
@perl -pi -e s,BitcoinTxInfo,BitcoinTxInfo2,g ./abi/WalletCoordinator.go
endef
define fix_maintainer_proxy_collision
@perl -pi -e s,BitcoinTxUTXO,BitcoinTxUTXO2,g ./abi/MaintainerProxy.go
@perl -pi -e s,BitcoinTxProof,BitcoinTxProof2,g ./abi/MaintainerProxy.go
@perl -pi -e s,BitcoinTxInfo,BitcoinTxInfo3,g ./abi/MaintainerProxy.go
endef

define after_contract_hook
$(eval type := $(1))
$(if $(filter $(type),MaintainerProxy),$(call fix_maintainer_proxy_contract_collision))
endef
define fix_maintainer_proxy_contract_collision
@perl -pi -e s,BitcoinTxUTXO,BitcoinTxUTXO2,g ./contract/MaintainerProxy.go
@perl -pi -e s,BitcoinTxProof,BitcoinTxProof2,g ./contract/MaintainerProxy.go
@perl -pi -e s,BitcoinTxInfo,BitcoinTxInfo3,g ./contract/MaintainerProxy.go
@perl -pi -e s,BitcoinTxUTXO,BitcoinTxUTXO2,g ./cmd/MaintainerProxy.go
@perl -pi -e s,BitcoinTxProof,BitcoinTxProof2,g ./cmd/MaintainerProxy.go
@perl -pi -e s,BitcoinTxInfo,BitcoinTxInfo3,g ./cmd/MaintainerProxy.go
endef
Comment on lines +36 to +47
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we need this piece. Can you elaborate, please?

Contract bindings are generated based on the ABI. If we fixed the ABI in after_abi_hook, why the additional work?

Copy link
Member

Choose a reason for hiding this comment

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

I was about to ask the same question 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I comment this function and run make mainnet, I'm getting compilation errors like this:

pkg/chain/ethereum/tbtc/gen/contract/MaintainerProxy.go:730:3: cannot use arg_mainUtxo (variable of type "github.com/keep-network/keep-core/pkg/chain/ethereum/tbtc/gen/abi".BitcoinTxUTXO) as type "github.com/keep-network/keep-core/pkg/chain/ethereum/tbtc/gen/abi".BitcoinTxUTXO2 in argument to mp.contract.NotifyMovingFundsBelowDust

It seems the generated contract bindings still contain the wrong type (BitcoinTxUTXO instead of BitcoinTxUTXO2):

func (mp *MaintainerProxy) NotifyMovingFundsBelowDust(
	arg_walletPubKeyHash [20]byte,
	arg_mainUtxo abi.BitcoinTxUTXO,

	transactionOptions ...chainutil.TransactionOptions,
) (*types.Transaction, error) {
....
}

Copy link
Contributor Author

@tomaszslabon tomaszslabon Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Contract bindings are generated based on the ABI. If we fixed the ABI in after_abi_hook, why the additional work?

In after_abi_hook we change *.go files.
It seems the contract bindings are generated based on *.abi files.
The $< argument in this invocation means the first prerequisite will be used:

contract/%.go cmd/%.go: abi/%.abi abi/%.go _address/% ${artifacts_dir}/%.json
	$(info $* - generating Keep bindings)
	@go run github.com/keep-network/keep-common/tools/generators/ethereum $< contract/$*.go cmd/$*.go

which will be MaintainerProxy.abi and not MaintainerProxy.go.

Therefore, we either leave the code as is or we remove it and change MaintainerProxy.abi instead.

Copy link
Member

Choose a reason for hiding this comment

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

Let me re-check my understanding of the issue.

after_abi_hook renames structures that were incorrectly re-declared in the same Go package given this bug: ethereum/go-ethereum#24627.

This is the version on main:

keep-core/pkg/chain/ethereum/tbtc/gen/Makefile

Lines 24 to 26 in 78da87c

define fix_wallet_coordinator_collision
@perl -pi -e s,BitcoinTxInfo,BitcoinTxInfo2,g ./abi/WalletCoordinator.go
endef

My understanding of the bug, is that both the generated tbtc/gen/abi/Bridge.go and tbtc/gen/abi/WalletCoordinator.go declares BitcoinTxInfo struct. To make the Go compiler happy, we rename BitcoinTxInfo to BitcoinTxInfo2 in tbtc/gen/abi/WalletCoordinator.go.

Those two Go files are generated with @go run github.com/ethereum/go-ethereum/cmd/abigen.

Now, we add another contract that also uses BitcoinTx.Info in Solidity so it will have BitcoinTxInfo re-declared in the Go code given the issue from ethereum/go-ethereum#24627. We do the same trick as in main. We extend the after_abi_hook and do some renames:

keep-core/pkg/chain/ethereum/tbtc/gen/Makefile

Lines 26 to 33 in 36c0c90

define fix_wallet_coordinator_collision
@perl -pi -e s,BitcoinTxInfo,BitcoinTxInfo2,g ./abi/WalletCoordinator.go
endef
define fix_maintainer_proxy_collision
@perl -pi -e s,BitcoinTxUTXO,BitcoinTxUTXO2,g ./abi/MaintainerProxy.go
@perl -pi -e s,BitcoinTxProof,BitcoinTxProof2,g ./abi/MaintainerProxy.go
@perl -pi -e s,BitcoinTxInfo,BitcoinTxInfo3,g ./abi/MaintainerProxy.go
endef

This makes sense to me up to this point.

But this is not enough! If we try to build the project, we will encounter errors like:

pkg/chain/ethereum/tbtc/gen/contract/MaintainerProxy.go:730:3: cannot use arg_mainUtxo (variable of type "[github.com/keep-network/keep-core/pkg/chain/ethereum/tbtc/gen/abi](http://github.com/keep-network/keep-core/pkg/chain/ethereum/tbtc/gen/abi)".BitcoinTxUTXO) as type "[github.com/keep-network/keep-core/pkg/chain/ethereum/tbtc/gen/abi](http://github.com/keep-network/keep-core/pkg/chain/ethereum/tbtc/gen/abi)".BitcoinTxUTXO2 in argument to mp.contract.NotifyMovingFundsBelowDust

Looking at the generated code at the problematic line:

transaction, err := mp.contract.NotifyMovingFundsBelowDust(
  transactorOptions,
  arg_walletPubKeyHash,
  arg_mainUtxo,
)

mp.contract.NotifyFundsBelowDust is declared in the tbtc/gen/abi/MaintainerProxy.go and expects BitcoinTxUTXO2.

arg_mainUtxo from MaintainerProxy.go has an incorrect type though: arg_mainUtxo abi.BitcoinTxUTXO,

OK, that makes sense. Just like tbtc/gen/abi/MaintainerProxy.go this file is generated based on tbtc/gen/abi/MaintainerProxy.abi and in the *.abi file we don't have the rename.

In other words, the after_contract_hook is partially a result of ethereum/go-ethereum#24627 and partially how our Go contract bindings work. The fix from ethereum/go-ethereum#24924 will change the generated tbtc/gen/abi/*.go files but we'll have to adjust the @go run github.com/keep-network/keep-common/tools/generators/ethereum logic to follow the same naming rules as the *.abi file will remain unchanged.

Does it make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pdyraga Yes, it makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me as well!

Copy link
Member

Choose a reason for hiding this comment

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

@tomaszslabon Can you please add a comment to after_contract_hook referencing keep-network/keep-common#117 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 800fdda.


include ../../common/gen/Makefile
Empty file added pkg/chain/ethereum/tbtc/gen/_address/MaintainerProxy
View file
Open in desktop
Empty file.
2,383 changes: 2,383 additions & 0 deletions pkg/chain/ethereum/tbtc/gen/abi/MaintainerProxy.go
View file
Open in desktop

Large diffs are not rendered by default.

Loading