search

hermeznetwork / hermez-node

Hermez node Go implementation
GNU Affero General Public License v3.0
58 stars 33 forks source link

Add public marshalers and unmarshalers for API requests and responses #1030

Closed arnaubennassar closed 3 years ago

arnaubennassar commented 3 years ago

Rationale

Right now anyone trying to integrate with the node in Go has to built custom logic to parse request/response bodies from/to the API (including the SDK), this forces client to have some sort of duplicated structs, instead of using the ones defined in this repo.

This is specially relevant for the structs used in POST methods requests. Although they are already implemented (common.PoolL2Tx, common.AtomicGroup) except for common.CreateAccountAuth.

Note that there are lot's of such structs, and probably it will be overkill to implement this in a single PR

jeffprestes commented 3 years ago

The issue (related to https://github.com/hermeznetwork/hermez-go-sdk/issues/13) in Hermez Go SDK needs that this issue has been completed to be fixed.

jeffprestes commented 3 years ago

All MarshalJSON needs to return a public common struct instead of interface{}

mfcastellani commented 3 years ago

@jeffprestes and @arnaubennassar after some time looking at the structures present in the common package, I would like to discuss with you a way to standardize and improve the description of some of the structures in the package.

I'll start with PoolL2Tx. If this is something that will be used outside the project, that will be exposed, the name is very bad. It is not a pool, but a single transaction. Also, I don't see another type of transaction being externalized in our common package, so this structure could simply be called a transaction. This should extend to our api, so we would have a GET transaction endpoint that would receive an id and take care of fetching it inside the system, no matter where it is.

Another point is the way things are organized in files. We have for example Nonce, which is defined inside account. And it is used within pooll2tx.go. If Nonce is a reusable data type I believe it should have its own file inside common.

Another thing that caught my attention is that there are several "builders" such as NonceFromBytes or AccountFromBigInts that are not linked to any structure. Taking an example from Geth, if you want to call the Dial method to create a client you do ethclient.Dial(URL). In our case everything is within common. I don't know if it would be over-precious to have something like common.account.Account or common.account.FromBigInts, which in my opinion would be more descriptive and simpler to use.

I am raising these points since the objective of this PR is to have a better usability of the structures within the common, as said in this PR and related ones. And just bringing the json serialize and deserialize methods over here isn't going to make this API any prettier.

I would like your opinion. In time, I have already started to make some of these changes.

jeffprestes commented 3 years ago

I think this is a good discussion and I also think some structures names and functions should be renamed. However, I think we need to break these changes in issues to allow us to define the best moments to pay these technical debts. A general issue for this huge change will complicate the task management and tests will be less efective.

jeffprestes commented 3 years ago

Regarding the second topic, I think the approach to have these functions working independently works fine and in GETH project, for example, they have them it in several places under common package and they are useful and easy to use. Another advantage is that I don't need to initiate a variable to use them.

jeffprestes commented 3 years ago

Thanks for improving the api package and removing the interface{} . For sure it's the more urgent topic and your PR will remove limitations in the hermez-node usages.

mfcastellani commented 3 years ago

Regarding the second topic, I think the approach to have these functions working independently works fine and in GETH project, for example, they have them it in several places under common package and they are useful and easy to use. Another advantage is that I don't need to initiate a variable to use them.

No need to initiate a variable to run, that's the reason of existence of the packages.

But, talk is cheap, please take a look when you have time @jeffprestes

https://github.com/hermeznetwork/hermez-node/pull/1042/files

arnaubennassar commented 3 years ago

I don't see another type of transaction being externalized in our common package, so this structure could simply be called a transaction

We use L1Tx and L2Tx in other packages (db, synchronizer). We don't use this types in the API because we only have GET methods for those, and the response object has extended information (the same happens in common.PoolL2Tx, it's used for POST but not for GET). Probably the SDK will use L1Tx when it supports deposits, force exits, ... Apart from that, PoolL2Tx and L2Tx are also very different things, it's not just a way to say this one is forged and this one not. Once txs are sent to Ethereum a lot of information is lost so we cannot synchronize it and so we need a different struct to represent the information we get from on chain data (the lost information is passed to Ethereum through the ZKP, we need this info to build the ZKPs but we cannot recover it that's why we have to different structs)

With that being said, I don't like the name either the idea was to indicate that "this is a transaction from the pool", but the name is ugly and missleading