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.

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

Described transactions #4430

Merged
merged 16 commits into from
Apr 5, 2022
Merged

Conversation

ricmoo
Copy link
Contributor

@ricmoo ricmoo commented Nov 7, 2021

Adding an EIP for described transactions, splitting EIP-3224 into multiple EIPs as discussed on the discussions to.

(the EIP number in the draft will be updated once this PR has been assigned a number and this PR updated)

@eth-bot
Copy link
Collaborator

eth-bot commented Nov 7, 2021

All tests passed; auto-merging...

(pass) eip-4430.md

classification
updateEIP
  • passed!

Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

Since this is an ERC I'll leave it to others to review, but figured I would give some initial feedback to limit the time this EIP is stuck blocked.

EIPS/eip-4430.md Outdated Show resolved Hide resolved
EIPS/eip-4430.md Show resolved Hide resolved
EIPS/eip-4430.md Outdated Show resolved Hide resolved
EIPS/eip-4430.md Outdated Show resolved Hide resolved
@axic
Copy link
Member

axic commented Nov 11, 2021

Why did you wanted to revive 3224 if this is the split out version?

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
@ricmoo
Copy link
Contributor Author

ricmoo commented Nov 12, 2021

@axic This (EIP-4430) is the described transactions split out from EIP-3224 , which is now just the described messages. Once these are through, I'll be adding two new EIPs for the respective JSON-RPC methods.

EIPS/eip-4430.md Outdated Show resolved Hide resolved
@ricmoo
Copy link
Contributor Author

ricmoo commented Dec 6, 2021

Is there still anything outstanding on this EIP that I've missed?

@ricmoo
Copy link
Contributor Author

ricmoo commented Dec 27, 2021

@axic @MicahZoltu Any remaining issues regarding this?

I'm working on an article to help articulate this EIP and EIP-3224, but waiting for this to be posted to eips.ethereum.org.

Thanks!

@MicahZoltu
Copy link
Contributor

The remaining issue is lack of EIP editors. 😖 @lightclient and @axic are the only two people actively reviewing ERCs, and sometimes they both disappear for weeks on end. 😢

This is the problem with volunteer work...

@MicahZoltu
Copy link
Contributor

Maybe @wschwab wants to take a stab at reviewing this?

@ricmoo
Copy link
Contributor Author

ricmoo commented Jan 21, 2022

Pinging @wschwab

@wschwab
Copy link
Contributor

wschwab commented Jan 23, 2022

sorry, I'd missed this - will take a look (prolly not today, but within the next couple/few)

@ricmoo
Copy link
Contributor Author

ricmoo commented Jan 23, 2022

@wschwab Thanks; if you would like access to my draft article which aims to better explain this EIP, please DM me on twitter or send me an e-mail to me@ricmoo.com. :)

Copy link
Contributor

@wschwab wschwab left a comment

Choose a reason for hiding this comment

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

I've left some comments and thoughts - it looks like there are a couple of things that need cleaning up, but this is otherwise pretty much there!

Some of the comments may stem from my own misunderstanding - I do want to ping you for the article, but figured it would be better if I got things rolling in the meantime.

are generated simultaneously by evaluating the method on a contract:

```solidity
function eipXXXDescribe(bytes inputs, bytes32 reserved) view returns (string description, bytes execcode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function eipXXXDescribe(bytes inputs, bytes32 reserved) view returns (string description, bytes execcode)
function eip4430Describe(bytes inputs, bytes32 reserved) view returns (string description, bytes execcode)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I don't fully understand the spec. Is there a function actually named eip4430Describe that needs to be present in the contract in order to leverage this EIP? I suspect not since there could only be one per contract (or per argument set?) whereas the contract may contain many, if so, what is the specification for the name (and arguments) of a EIP-4430 compliant function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I want to avoid giving it a real name, since people might start using it before its ready and I want to avoid a signTypeData_v4 situation.

- Homoglyphs attacks
- Right-to-left mark may affect rendering
- Many other things, deplnding on your environment

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend reiterating the concern the point made above that contracts can implement this maliciously (purposely giving an inaccurate description)

EIPS/eip-4430.md Outdated
Comment on lines 124 to 126
@TODO (consider adding it as one or more files in `../assets/eip-####/`)

I will add examples in Solidity and JavaScript.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reference Implementation is optional, so this could be merged without it, but I do feel like it would be very useful for this EIP, and also figure you didn't want to leave this section quite like this - if you'd like you can still leave the latter line, but the @TODO should be removed imho

EIPS/eip-4430.md Outdated
title: Described Transactions
description: A technique for contract authors to enable wallets to provide a human-readable description of the effect of of a transaction with a given contract.
author: Richard Moore (@ricmoo), Nick Johnson (@arachnid)
discussions-to: https://github.com/ethereum/EIPs/issues/4431
Copy link
Contributor

Choose a reason for hiding this comment

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

the usual recommendation is to link to a thread in the Ethereum Magicians Discourse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I change it? There is already some (but not much discussion) on the current link.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, all new EIPs should be using Ethereum Magicians. Since this is a new EIP, it should as well. We currently only make exceptions for existing EIPs (merged as draft before we moved to Ethereum Magicians) that are receiving updates or moving through status changes.

@github-actions github-actions bot added the stale label Mar 25, 2022
@ricmoo
Copy link
Contributor Author

ricmoo commented Mar 30, 2022

Is it possible to move this to draft soon-ish? I'd rather not go through the effort of un-stagnating an EIP. Once it is draft, it is much easier to make changes to.

@github-actions github-actions bot removed the stale label Mar 30, 2022
@ricmoo
Copy link
Contributor Author

ricmoo commented Mar 31, 2022

I've made the requested change and will link the old discussions to the new.

@MicahZoltu
Copy link
Contributor

CI failures:

eip-4430.md: description exceeds max length of 140 characters

./EIPS/eip-4430.md:15: funcions ==> functions

EIPS/eip-4430.md Outdated Show resolved Hide resolved
ricmoo and others added 2 commits March 31, 2022 19:56
@MicahZoltu MicahZoltu closed this Apr 1, 2022
@MicahZoltu MicahZoltu reopened this Apr 1, 2022
@MicahZoltu
Copy link
Contributor

Oh, right. I don't have permission to approve ERCs. You'll have to wait for one of the ERC editors.

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

LGTM

@kodiakhq kodiakhq bot merged commit 904be25 into ethereum:master Apr 5, 2022
PowerStream3604 pushed a commit to PowerStream3604/EIPs that referenced this pull request May 19, 2022
* Updated EIP-2098 compact representation with suggestions and moved to review.

* Added draft for described transactions.

* Updated EIP-4430 with its EIP number and discussions-to.

* Updated EIP-4430 filename.

* Fixed per request.

* Update EIPS/eip-4430.md

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>

* Fixed typo.

* Updated as requested.

* Updated as per suggestion.

* Revert file modified in wrong branch.

* Updated link to etheruem magicians.

* Update EIPS/eip-4430.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Updated EIP-4430 with shorter description.

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
Co-authored-by: Micah Zoltu <micah@zoltu.net>
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* Updated EIP-2098 compact representation with suggestions and moved to review.

* Added draft for described transactions.

* Updated EIP-4430 with its EIP number and discussions-to.

* Updated EIP-4430 filename.

* Fixed per request.

* Update EIPS/eip-4430.md

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>

* Fixed typo.

* Updated as requested.

* Updated as per suggestion.

* Revert file modified in wrong branch.

* Updated link to etheruem magicians.

* Update EIPS/eip-4430.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Updated EIP-4430 with shorter description.

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
Co-authored-by: Micah Zoltu <micah@zoltu.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants