ExocoreNetwork / exocore

Omnichain Restaking
7 stars 9 forks source link

fix: error retruned by precompiled contract can not be caught with tr… #69

Closed adu-web3 closed 4 months ago

adu-web3 commented 4 months ago

Description


We have many precompiled contracts, and when we call the functions of precompiled contracts in EVM, if precompiled contract returns an error, this error can not be caught neither by try/catch clause nor low level call clause. A workaround is that we return the false value to indicate that the function of precompiled contract failed instead of return an error.

adu-web3 commented 4 months ago

@mikebraver @bwhour , after talking with @TimmyExogenous , we think these three modules assets, deposit and withdraw should be unified as one module. withdraw and deposit are both actions on assets, we shouldn't have separate modules for them. And for the interface of withdrawing reward, I think it belongs to asset module as well

adu-web3 commented 4 months ago

some tests have failed and i'm fixing failed tests. Besides we might need to consider whether or not to take deposit failure as protocol exception or tolerable error: https://github.com/ExocoreNetwork/exocore-contracts/issues/35

adu-web3 commented 4 months ago

I have tested with localnet and this should fix the error, @cae1um could you help test if this works? I would fix the failed tests reported by github CI

cae1um commented 4 months ago

I have tested with localnet and this should fix the error, @cae1um could you help test if this works? I would fix the failed tests reported by github CI

okay, I will

cloud8little commented 4 months ago

Test passed with positive deposit/delegation/undelegateFrom/withdrawPrinciple with exocoreGateway contract in https://github.com/ExocoreNetwork/exocore-contracts/pull/44/commits/3aadd63925d805ab4d509b280c4e001fdc3f56f3.
also negative withdraw also will unblock the layerzero messaging. (with NonShortCircuitEndpointV2Mock )

MaxMustermann2 commented 4 months ago

Can you add the following patch for client chains precompile?

diff --git a/precompiles/clientchains/tx.go b/precompiles/clientchains/tx.go
index 684ca0a..9ce49bd 100644
--- a/precompiles/clientchains/tx.go
+++ b/precompiles/clientchains/tx.go
@@ -15,11 +15,21 @@ func (p Precompile) GetClientChains(
    args []interface{},
 ) ([]byte, error) {
    if len(args) > 0 {
-       return nil, errorsmod.Wrapf(assetstypes.ErrInvalidInputParameter, "no input is required")
+       ctx.Logger().Error(
+           "GetClientChains",
+           "err", errorsmod.Wrapf(
+               assetstypes.ErrInvalidInputParameter, "no input is required",
+           ),
+       )
+       return method.Outputs.Pack(false, nil)
    }
    infos, err := p.assetsKeeper.GetAllClientChainInfo(ctx)
    if err != nil {
-       return nil, err
+       ctx.Logger().Error(
+           "GetClientChains",
+           "err", err,
+       )
+       return method.Outputs.Pack(false, nil)
    }
    ids := make([]uint16, 0, len(infos))
    for id := range infos {
@@ -27,9 +37,13 @@ func (p Precompile) GetClientChains(
        // based it on uint16, so we have to stick with it.
        // TODO: change it to uint32 here and in other precompiles.
        if id > math.MaxUint16 {
-           return nil, errorsmod.Wrapf(
-               assetstypes.ErrInvalidInputParameter, "client chain id is too large",
+           ctx.Logger().Error(
+               "GetClientChains",
+               "err", errorsmod.Wrapf(
+                   assetstypes.ErrInvalidInputParameter, "client chain id is too large",
+               ),
            )
+           return method.Outputs.Pack(false, nil)
        }
        // #nosec G701 // already checked
        convID := uint16(id)
adu-web3 commented 4 months ago

I have tested with localnet and this should fix the error, @cae1um could you help test if this works? I would fix the failed tests reported by github CI

okay, I will @cae1um Sorry I intended to inform @cloud8little. @cloud8little has tested it, anyway thank you :)

adu-web3 commented 4 months ago

Can you add the following patch for client chains precompile?

diff --git a/precompiles/clientchains/tx.go b/precompiles/clientchains/tx.go
index 684ca0a..9ce49bd 100644
--- a/precompiles/clientchains/tx.go
+++ b/precompiles/clientchains/tx.go
@@ -15,11 +15,21 @@ func (p Precompile) GetClientChains(
  args []interface{},
 ) ([]byte, error) {
  if len(args) > 0 {
-     return nil, errorsmod.Wrapf(assetstypes.ErrInvalidInputParameter, "no input is required")
+     ctx.Logger().Error(
+         "GetClientChains",
+         "err", errorsmod.Wrapf(
+             assetstypes.ErrInvalidInputParameter, "no input is required",
+         ),
+     )
+     return method.Outputs.Pack(false, nil)
  }
  infos, err := p.assetsKeeper.GetAllClientChainInfo(ctx)
  if err != nil {
-     return nil, err
+     ctx.Logger().Error(
+         "GetClientChains",
+         "err", err,
+     )
+     return method.Outputs.Pack(false, nil)
  }
  ids := make([]uint16, 0, len(infos))
  for id := range infos {
@@ -27,9 +37,13 @@ func (p Precompile) GetClientChains(
      // based it on uint16, so we have to stick with it.
      // TODO: change it to uint32 here and in other precompiles.
      if id > math.MaxUint16 {
-         return nil, errorsmod.Wrapf(
-             assetstypes.ErrInvalidInputParameter, "client chain id is too large",
+         ctx.Logger().Error(
+             "GetClientChains",
+             "err", errorsmod.Wrapf(
+                 assetstypes.ErrInvalidInputParameter, "client chain id is too large",
+             ),
          )
+         return method.Outputs.Pack(false, nil)
      }
      // #nosec G701 // already checked
      convID := uint16(id)

sure, done in new commit