Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add overriding name to method spec so it comes through in contract json #550

Merged
merged 9 commits into from
Oct 7, 2022
Merged

Add overriding name to method spec so it comes through in contract json #550

merged 9 commits into from
Oct 7, 2022

Conversation

barnjamin
Copy link
Contributor

@barnjamin barnjamin commented Oct 5, 2022 •
edited
Loading

Sometimes we want to have the same named method with different arguments, this should be present in the contract spec not just the handler method signature.

Its already the case that the method signature will get its name overridden but if we don't pass through the overriding name to the contract.json output, calls to the contract will fail with an invalid signature since they don't match.

ahangsu
ahangsu reviewed Oct 5, 2022
View reviewed changes
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

Looks good, I was wondering another set of API level problem:
We should provide overriding_name as an (optional) arg in constructing an ABIReturnSubroutine, rather than using overriding_name as an arg in method_spec and method_signature.

@ahangsu ahangsu mentioned this pull request Oct 5, 2022
Merged
ahangsu and others added 3 commits October 5, 2022 13:00
@ahangsu
Proposing API minor change on ABIReturnSubroutine for name overridi…
dc8e6ee 
…ng (#551)

* Typo Fix for `abi.Uint64TypeSpec` (#549)

* minor suggestion on ABIReturnSubroutine API change
@barnjamin
make tests pass
758f520
@barnjamin
Merge branch 'master' into override-name
a20533e
ahangsu
ahangsu approved these changes Oct 5, 2022
View reviewed changes
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

Looks good to me, would be good if you add warning or comments in method_selector in ABIReturnSubroutine about the future API change.

@michaeldiamant michaeldiamant mentioned this pull request Oct 7, 2022
Merged
@ahangsu ahangsu mentioned this pull request Oct 7, 2022
Merged
@ahangsu @michaeldiamant
ABIReturnSubroutine doc/test-case on name_override decorator (#557)
f2b2cea 
* abi overridden name subr doc testcase

* minor typo fix

Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>

Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>
michaeldiamant
michaeldiamant approved these changes Oct 7, 2022
View reviewed changes
Copy link
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@barnjamin Thanks for addressing the inconsistent name override behavior. ☕

@michaeldiamant
Copy link
Contributor

@barnjamin Sorry - Post-approval note: Appreciate a CHANGELOG update prior to merging.

@barnjamin barnjamin merged commit 33a4e5e into master Oct 7, 2022
@barnjamin barnjamin deleted the override-name branch October 7, 2022 18:31
@ahangsu ahangsu mentioned this pull request Oct 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahangsu ahangsu ahangsu approved these changes

@michaeldiamant michaeldiamant michaeldiamant approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@barnjamin @michaeldiamant @ahangsu