kangarang / tcr-ui

A client-side shell to interact with token-curated registries
MIT License
68 stars 28 forks source link

Apply called with wrong number of arguments #107

Closed d-xo closed 6 years ago

d-xo commented 6 years ago

apply is called with four arguments (here), but the contract expects three arguments (here).

This meant that all calls to apply would fail for the TCR that I deployed.

If I remove the listingID argument from the call to apply and update the contract ABI's to match (e.g. 0xafbbefb788e478d30f2aca7c8fc8b8271a9603a2), then the calls to apply succeed (see this commit)

Is this UI repo designed to work against an older version of the contracts? Am I missing something? I'm totally happy to submit a PR fixing this, if it is indeed a bug 🙂

kangarang commented 6 years ago

@xwvvvvwx currently this UI defaults to supporting the contracts compiled from this forked tcr repo. prototyping with that forked repo is convenient for faster iterations (e.g. the minDeposit issue). the diff in the contracts is the additional apply argument and its corresponding _Application event emission. i put a notice on the readme, but i should've written a notice on this repo as well. my apologies for that lack of communication

previously, this ui would package the apply arguments and corresponding metadata, add them to ipfs, and then submit the resulting ipfs multihash as an argument to the actual apply function. appropriately that multihash would be emitted in the _Application event, with which the UI would use to retrieve the ipfs data resolved at that hash. here's some more details for those conventions

the reasons for the recent diff in the contracts was: (1) it was getting difficult to rely on infura's ipfs endpoint, and (2) a single data string didn't provide enough metadata for listing applications

wrt infura-ipfs: since the engine of this ui runs client-side, the ui must be able to build the context of the current state of the smart contracts at any given point (i.e. creating and updating listing objects). in order to work around the infura ipfs rate limit, i wrote a batching function, but it was still unbearable when dealing with a larger registry. to see what i mean, check out the console.logs at https://sunset-registry.firebaseapp.com/ -- switch to mainnet (which will autopoint to the adChain Registry) and you'll get an idea of how it batches the creation of listings

wrt listing-metadata: with 2 strings, we can at least prototype something simple like rendering images from urls, or providing more detailed descriptions for listing applications

on load, the ui still retrieves contract abis from ipfs, which sets up the app to send transactions and poll for changes. since the diff, it has been using this multihash to retrieve those new abis. a quick workaround for you to continue using the 3-arg-apply contract abis: in this function, return that multihash regardless of network. that multihash pulls the contract abis which are equipped to execute the 3-argument apply, and polls for the corresponding _Application event logs topics

i think using ipfs for generating listing applications is still the way to go in the long run, and surely the convention to use on the mainnet. however since this ui is still very useful for prototyping, i made those changes to quickly work around the immediate constraints

kangarang commented 6 years ago

ah, just saw more of that commit: https://github.com/humansTCR/tcr-ui/commit/da484c6ff39c5e03cf923a2374dd14f87e4f9695. looks good to me!

d-xo commented 6 years ago

OK clear. Thanks for the detailed answer!