MaintainerProxy bindings #3630
MaintainerProxy bindings #3630
Conversation
tomaszslabon
force-pushed
the
maintainer-proxy-bindings
branch
from
95355e9
to
9f2a249
Compare
tomaszslabon
force-pushed
the
maintainer-proxy-bindings
branch
from
9f2a249
to
9dc0b77
Compare
tomaszslabon
requested review from
pdyraga,
nkuba and
lukasz-zimnoch
and removed request for
nkuba
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I was about to ask the same question 😀
There was a problem hiding this comment.
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) {
....
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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
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?
There was a problem hiding this comment.
@pdyraga Yes, it makes sense.
There was a problem hiding this comment.
Makes sense to me as well!
There was a problem hiding this comment.
@tomaszslabon Can you please add a comment to after_contract_hook referencing keep-network/keep-common#117 ?
There was a problem hiding this comment.
Done in 800fdda.
#Depends on #3630. This PR adds a handle to the MaintainerProxy contract in the Tbtc chain.
This PR adds binding generation for the
MaintainerProxycontract.The bindings were generated using the
make mainnetcommand.