Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

add blurb in @truffle/contract README about overloaded functions #5836

Merged
merged 3 commits into from Jan 23, 2023

Conversation

eggplantzzz
Copy link
Contributor

No description provided.

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some suggestions for your consideration.

packages/contract/README.md Outdated Show resolved Hide resolved
packages/contract/README.md Outdated Show resolved Hide resolved
packages/contract/README.md Outdated Show resolved Hide resolved
```

Truffle contract (internally using web3's overloaded function resolution) will see that you input a string and
call the second method in the contract to set `myString`. Again however, sometimes when your contract is more
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahaha you wish! Web3's overload resolution only works reliably when the functions have different numbers of arguments. The behavior you describe here is obviously what ought to actually happen, but not necessarily what actually will. In reality there's a decent chance that Web3 tries to go with the integer function anyway and then just errors on you because the input wasn't an integer like expected. This is why I keep saying it's so bad, and the primary reason I created Truffle Encoder with the hope of replacing this!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that is terrible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I didn't realize it was primarily based on number of arguments. Bummer, I'll update this blurb here. I wonder how easy it would be to create our own shim/adapter-thingy for this...

Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna have to hit request changes, the example needs to be changed (see my comment). Might also be good to adjust the language earlier to account for this.

Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems OK to me! There should probably also be a corresponding change to trufflesuite.com.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants