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

Remove error string from transfer. Replace with constant string #777

Closed
3 tasks
colin-axner opened this issue Jan 20, 2022 · 3 comments · Fixed by #818
Closed
3 tasks

Remove error string from transfer. Replace with constant string #777

colin-axner opened this issue Jan 20, 2022 · 3 comments · Fixed by #818
Assignees
Labels
20-transfer type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.

Comments

@colin-axner
Copy link
Contributor

colin-axner commented Jan 20, 2022

Summary

Any values included in an acknowledgement will be committed in the state machine. Thus the error message returned from OnRecvPacket is state machine breaking if it changes. This makes our code hard to change in patch releases (and this restriction applies to our dependencies). We should remove the err.Error() from the transfer ack and replace with a constant error string that won't change

see #744 for more information


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@crodriguezvega crodriguezvega moved this to Backlog in ibc-go Jan 21, 2022
@crodriguezvega crodriguezvega added the type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. label Jan 21, 2022
@crodriguezvega crodriguezvega moved this from Backlog to Todo in ibc-go Jan 21, 2022
@colin-axner
Copy link
Contributor Author

The abci error code can be emitted in the error acknowledgement.

@damiannolan damiannolan self-assigned this Jan 31, 2022
@damiannolan
Copy link
Member

The abci error code can be emitted in the error acknowledgement.

This refers to just ics27, right? Transfer just requires a string const here.

NewErrorAcknowledgement() in 04-channel/types should probably be documented to specify a deterministic error string is required. Even still, we can't guarantee that developers will provide that.

@colin-axner
Copy link
Contributor Author

colin-axner commented Feb 1, 2022

This refers to just ics27, right? Transfer just requires a string const here.

That string const being deterministic rests on the assumption that error strings are included in the state machine, which is not true. The ABCI code could be included in the same way it was done for ICS27 in #794

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
20-transfer type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants