Fix encoding of mixed dynamic/fixed arguments

[markspanbroek]

Encoding of mixed dynamic and fixed elements in a function call did not work. See test_encoding.nim for a new test that failed on the old implementation. This PR replaces the old implementation with the one from contractabi, which does encode correctly and comes with a full set of unit tests.

List of changes:

1. Adds test for encoding and decoding of mixed elements (test_encoding.nim)
2. Replaces encoding and decoding implementation with the one from contractabi
3. Extracts most of the logic in the contract macro into smaller functions

[emizzle]

All looks good to me 👍

Maybe one thing that would be useful in the PR description for posterity's sake is what the benefits are of using nim-contract-abi instead of the encoder/decoder that is currently there. Maybe there could be a test included that highlights this improvement. Not necessary, just a suggestion.

Also, before merging, might be useful to squash commits to keep the history clean.

[markspanbroek]

Thanks for the review @emizzle! I'll address your comments on Monday.

[zah]

I'd like to review these changes in depth as well. Not so long ago, I've prepared some fuzzing tests for the library that can be added to this branch in order to make sure that the new code is safe/correct before merging it. Downstream projects can reference this branch directly in case the functionality is needed urgently.

[markspanbroek]

Maybe one thing that would be useful in the PR description for posterity's sake is what the benefits are of using nim-contract-abi instead of the encoder/decoder that is currently there. Maybe there could be a test included that highlights this improvement. Not necessary, just a suggestion.

I updated the description of this PR to highlight the benefits of using contractabi, and to point out which test highlights the improvement.

[arnetheduck]

why not put contractabi in this repo? ie it's within the domain of this library already, and doing so simplifies using the library in general.

[markspanbroek]

why not put contractabi in this repo?

Main reason is dependencies. In Dagger/Codex we only need the encoding scheme of contractabi for off-chain communication (in nitro state channels and storage marketplace requests/bids). Depending on web3 and its many sub-dependencies isn't desirable in that situation.

[arnetheduck]

Main reason is dependencies.

this is a bit of a self-made problem and having it separate contributes to that world - once you start creating one-function-libraries, it's a mess to depend on them as well - the overhead of any change goes up exponentially, specially if that change is a cross-repo change as often happens with tightly related libraries like web3+abi - a simple fix becomes a nightmare of dependency resolution and juggling - it could be worth exploring whether it makes sense to move some code around from the existing dependencies such that nim-web3 itself can become more standalone and therefore easier to use, instead of further aggravating the problem.

[markspanbroek]

That's not quite what I meant, I'll try to explain. I don't mind depending on a lot of small libraries, that's what a package manager is for. I do mind depending on functionality that I do not need. For instance, web3 indirectly depends on native rocksdb. When I only want to use abi encoding, I do not want to solve problems relating to rocksdb, because byte encoding doesn't need rocksdb.

Coordinating cross-library changes is a bit of a separate problem. I don't think it's been solved to everyone's satisfaction yet, seeing how everyone in the javascript world is still arguing about monorepos versus multi-repo versus monoliths :)

[arnetheduck]

I don't mind depending on a lot of small libraries, that's what a package manager is for.

pragmatically, we're lacking this - which is why we avoid solutions that explicitly require one, most of the time

For instance, web3 indirectly depends on native rocksdb.

Indeed - this is a problem, and what I'm suggesting is that instead of making up workarounds such as making inconveniently and unnaturally small libraries that de facto belong to an existing library due to their tight co-dependence, a better strategy might be to address the root cause of why nim-web3 pulls in dependencies that are not relevant to its function, and address that root cause by making the dependency list of nim-web3 smaller - regardless if you believe in monorepos or not, this is a better outcome over-all, from what I've seen here (nim-web3 has picked up a considerable amount of cruft over the years).

For dagger, it might be easier to include yet another micro-library instead of nim-web3, but for everyone else already using nim-web3, it's a cost, specially based on our current tooling situation.

[arnetheduck]

still arguing about monorepos versus multi-repo versus monoliths

this is also a cookie that you can easily both have and eat, by using a package manager that supports multiple libraries per repository - then you can have accurate and minimal dependencies and atomic multi-library changes conveniently.

but again, we don't have a package manager, ergo, small libraries have a much higher cost than in languages with a good package manager, ergo, we have to solve a different equation when weighing whether to start a new library or expand an existing one (this is also independent of what we might want to have in the future etc - in that beautiful future with a working package manager, we can split things up to our heart's desire, but that doesn't mean we should pay the costs right now)

[kdeme]

Additionally contractabi appears to use two more small libraries, questionable and upraises which I think are typically not (yet?) used in most of our code bases. So practically this brings 3 new libraries.

[emizzle]

Depending on web3 and its many sub-dependencies isn't desirable in that situation.

If we did decide to add nim-contract-abi in to nim-web3, it could be done in a way that it would not cause consumers to import unneeded deps.

When encoding.nim was first separated out in to its own module, it was for the sole purpose of allowing users to import the encoding module on its own without needing to import all of the other - unimported - dependencies. We did this in desktop. As an example, in a nimbus-build-system environment, running:

import
  web3/[encoding, ethtypes]

let x = FixedBytes[32].fromHex("0x60D0C2DAF1C10803DB5781D876CDBC42EADD52E6E49C2A1A7C1E5952B279E463")

echo x.encode

would only bring in `web3/[encoding, ethtypes] and any of their transitive dependencies, but it would not bring in other web3 deps, eg rocksdb.

[arnetheduck]

it could be done in a way that it would not cause consumers to import unneeded deps.

this is a good point - the nimble file is there mainly for CI but does not actually cause code dependencies - nim will not build rocksdb unless you actually use a module that transitively imports rocksdb.

in other words, for dagger you can just ignore that the deps are listed in the nimble file.

[markspanbroek]

When encoding.nim was first separated out in to its own module, it was for the sole purpose of allowing users to import the encoding module on its own without needing to import all of the other - unimported - dependencies.

Thanks for making this explicit @emizzle. I did indeed not foresee problems with the imports, but more with the declared dependencies themselves; they can conflict with other dependencies or simply fail to install. It is also good to know that the status desktop project has a use case for a separate encoding library.

[arnetheduck]

they can conflict with other dependencies or simply fail to install.

this is indeed why we don't use nimble - we also don't use version ranges in nimble files, because the nimble resolver itself is broken / buggy and cannot resolve them - we've been there and stopped doing so.

Long story short, I'd love to see these changes brought back to the nim-web3 repo without depending on contractabi - the introduction of the additional dependencies adds a maintenance overhead for current users of nim-web3 that seems unwarranted, and would prevent them from upgrading nim-web3 for unrelated reasons without significant work.

The right solution that benefits all here is to ensure that the dependencies of nim-web3 itself get fixed, which allows everyone to benefit, instead of providing an improvement for one (unreleased) project at cost for another (released) - this is a separate PR though than fixing the encoding.

[arnetheduck]

the nimble resolver is unable to deal with version ranges correctly - we don't use them because they eventually cause the CI of unrelated projects to fail - ie depending on "latest" is the lesser evil

[markspanbroek]

Closing this PR for now, due to the points raised by @arnetheduck. A cleanup of web3's dependencies is a bit too much work outside of my main priorities for me to pick up right now.

[arnetheduck]

Ideally we could move the corrected argument code into this repo as well, or at least document it in an issue - the dependency cleanup is an orthogonal thing, it can happen after the argument fix is in here. nim-web3 is already in production use, so we need to be a little bit careful about what dependencies are added to it - as you will have noticed, it's a lot harder to remove them than it is to add them.

[markspanbroek]

Ideally we could move the corrected argument code into this repo as well, or at least document it in an issue

For now, I've opened this issue: #45

[zah]

Er, what is the outcome of this discussion? @markspanbroek, did you decide to fork the nim-web3 code in your own project?

[markspanbroek]

did you decide to fork the nim-web3 code in your own project

Well, I guess technically I did, because currently daggercontracts is still pointing to the branch that I used for this PR :) Of course I don't intend to keep it that way. I need some time to figure out when and how to best approach this, but that branch will go away later.