mx-chain-go icon indicating copy to clipboard operation
mx-chain-go copied to clipboard

Add code-hash endpoint to address group

Open lcswillems opened this issue 2 years ago • 3 comments

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)

In Arda, we are going to add a service that tracks if smart contract binary codes change.

Currently, it is impossible to only request the code hash. The only way is to download all the account data (including the binary code that can be huge).

Proposed Changes

Add a code-hash endpoint to the address group.

Testing procedure

lcswillems avatar Aug 22 '22 16:08 lcswillems

@bogdan-rosianu @iulianpascalau I have forced pushed to add changes you requested and get the commit signed.

I think I fixed all the issues you reported except one about the following code in node.go:

}

codeHash := userAccount.GetCodeHash()
return string(codeHash), blockInfo, nil
  1. I don't understand what is wrong with this. I checked the code in nodeFacade.GetAccount() but don't understand why GetCode should appear in GetCodeHash.

  2. You told me to return an hex version of the code hash. I did the modification. However, it seems I should rather return a base64 version of the code hash, isn't it? Because in the endpoint /address/:address, the code hash is base64 encoded.

lcswillems avatar Aug 24 '22 11:08 lcswillems

@bogdan-rosianu @iulianpascalau I have forced pushed to add changes you requested and get the commit signed.

I think I fixed all the issues you reported except one about the following code in node.go:

}

codeHash := userAccount.GetCodeHash()
return string(codeHash), blockInfo, nil
  1. I don't understand what is wrong with this. I checked the code in nodeFacade.GetAccount() but don't understand why GetCode should appear in GetCodeHash.
  2. You told me to return an hex version of the code hash. I did the modification. However, it seems I should rather return a base64 version of the code hash, isn't it? Because in the endpoint /address/:address, the code hash is base64 encoded.

Let's see: the userAccount.GetCodeHash() returns the plain []byte slice (usually will be here 32 bytes). These will almost surely contain unreadable characters. Like /0 or /1 and so forth. If you just cast it to string, the webserver will think that this is a valid string and, I think, will not try to encode it even further. The suggestion to return the hex encoded code hash was one suggestion, but of course it can be encoded into a base64 form. To comply with the "clean code" guidance, indeed, you should do the encoding only on the most outer layer of the plugin involved and pass the code hash as []byte slice everywhere else (node.go, the facade, interfaces involved and so on). So, in addressGroup.go L245 you should put:

shared.RespondWithSuccess(c, gin.H{"codeHash": base64.StdEncoding.EncodeToString(codeHash), "blockInfo": blockInfo}) 

Or you can use hex. We mostly return hex encoded stuff as it is easier to handle on some frontends.

iulianpascalau avatar Aug 24 '22 13:08 iulianpascalau

Thanks a lot @iulianpascalau for the time you took to explain me! I added a new commit that should address the remaining issue

Thanks a lot @bogdan-rosianu for your review and time too!

lcswillems avatar Aug 24 '22 15:08 lcswillems

@iulianpascalau Two tests were failing. Sorry for that! I fixed them now.

lcswillems avatar Aug 25 '22 10:08 lcswillems

@bogdan-rosianu Thanks for the review. I removed the useless encoding

lcswillems avatar Aug 25 '22 14:08 lcswillems

Codecov Report

:exclamation: No coverage uploaded for pull request base (external-codehash-endpoint@65ccced). Click here to learn what that means. The diff coverage is n/a.

:exclamation: Current head 3f6de7b differs from pull request most recent head 99dd450. Consider uploading reports for the commit 99dd450 to get more accurate results

@@                      Coverage Diff                      @@
##             external-codehash-endpoint    #4391   +/-   ##
=============================================================
  Coverage                              ?   73.67%           
=============================================================
  Files                                 ?      678           
  Lines                                 ?    86911           
  Branches                              ?        0           
=============================================================
  Hits                                  ?    64032           
  Misses                                ?    18026           
  Partials                              ?     4853           

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

codecov-commenter avatar Aug 29 '22 09:08 codecov-commenter

Thank you!!

lcswillems avatar Aug 29 '22 09:08 lcswillems

Hi @iulianpascalau @bogdan-rosianu Has this PR merged in the codebase somewhere? The branch external-codehash-endpoint doesn't exist anymore. And the code of the PR doesn't seem to be in development or master.

lcswillems avatar Sep 26 '22 07:09 lcswillems

By the way, the product that would benefit from this endpoint has been launched. It is Arda Audits: https://arda.run/audits

lcswillems avatar Sep 26 '22 07:09 lcswillems

Hello. We've planned to phase out the development branch. The docs will be updated soon to specify where the external PRs should be started from. However, before phasing out the development branch, we've merged everything to rc/v1.4.0 since the release we've just rolled out (v1.3.42) was feature-frozen. TL;DR, the changes can be found on rc/v1.4.0 -> the upcoming planned release https://github.com/ElrondNetwork/elrond-go/blob/894a3d7c29c8b4f24030679ca46f5339466cadce/api/groups/addressGroup.go#L23

iulianpascalau avatar Sep 26 '22 07:09 iulianpascalau

Great @iulianpascalau ! Thank you for the precisions!

lcswillems avatar Sep 26 '22 08:09 lcswillems