pinknetworkx / atomicassets-contract

Smart Contract of the AtomicAssets standard.
MIT License
149 stars 88 forks source link

Abstracted the shared multi-index tables into a public header #26

Closed dallasjohnson closed 4 years ago

dallasjohnson commented 4 years ago

While working on some contract code for a wax game project I found we were duplicating some of your work by manually copying table struct definitions into out own header. This PR provides a way to have a common source of truth for the table structure definitions that could be used by the atomic-assets code and 3rd party consumers of the atomic-assets tables. With this change the 3rd party code would only need to import the public header and would have an up-to-date definition of all the table structures.

I also run clang formatter locally which has modified the formatting of the code. I can include that in the PR if your would like or I can go through and revert the formatting changes this has introduced,

jona-wilmsmann commented 4 years ago

Thanks for the pull request, providing an interface header is a great idea. There are some problems with your pull request:

Firstly, you deleted the custom dispatcher (the apply function). This custom dispatcher is however needed because it handles transfer notifications so that the smart contract can dynamically handle deposits from different token contracts that are not known when compiling.

Secondly, in my opinion an interface header should use structs instead of the eosio TABLE alias. By using structs, the table definitions are not included in the automatically generated ABI. That's especially important if your contract were to use the same table names (like balances or config, which are pretty common).

So instead of splitting the header file in two parts, I added a new file that is not used by the atomicassets contract and is specifically suited to be used as a header for other contracts https://github.com/pinknetworkx/atomicassets-contracts/commit/f2e37e54bf3a40240063c31e6bc7afbfbc8b9d83

Once again, thanks a lot for taking the time to write this pull request, it's a great idea!

dallasjohnson commented 4 years ago

Thanks for the feedback on the PR @jona-wilmsmann . Sorry, I had forgotten about the the abi generation on the tables once I removed them from the main contract. However I have now added the annotations to ensure they work as expected for the atomicassets contract and will not generate abi entries for the other contracts using the public header. The intention was not only to create an interface for others but also to ensure there was only one definition of the structs to maintain and therefore prevent them drifting out of sync. With the current changes, for example, there is now templates_s in one header and presets_s in the other which would be an easy mistake to make - also with some different field names such as template_id vs preset_id.

In regards to deleting the custom dispatcher this wasn't quite accidental. Although during my final merge conflict resolution before opening the PR I accidentally removed the [[eosio::on_notify("*::transfer")]] annotation which would ensured the transfer notifications would work as you expected .Sorry about that 🤦 . With the combination of ACTION annotations and the eosio::on_notify annotation, through my experience, you should not need the custom dispatcher anymore. Also, I think if you do have the custom dispatcher in the code it will use the custom one over the auto generated one but not a combination of both.

I'll open a new PR with the updated changes and see what you think.