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

BUG: (minor) Token ID Attributes Should Be Strings #507

Open
mccallofthewild opened this issue Mar 16, 2023 · 1 comment
Open

BUG: (minor) Token ID Attributes Should Be Strings #507

mccallofthewild opened this issue Mar 16, 2023 · 1 comment
Labels
ics-721 Possible protocol vulnerability improv

Comments

@mccallofthewild
Copy link
Contributor

Summary of Bug

Nothing crazy here. The newtype pattern's benefits do not extend to response attributes. Token ID attrs should be formatted such that transactions can be easily searched and indexed by token id.

Environment

  • OS: MacOS
  • Software version: cw-ics721-bridge

Steps to Reproduce

Steps to reproduce the behavior:
Transfer cw-ics721-bridge

Expected and Actual Behavior

Expected:

.add_attribute("token_id", String::from(token_id))
"value": "mytokenid"

Actual:

.add_attribute("token_id", token_id)
"value": "[TokenId(\"mytokenid\")]"

Error Stack

NA

Additional Context

NA

@mccallofthewild
Copy link
Contributor Author

Recommendation:

Use duplicate attributes to express batch transfers. There is no practical way for BigDipper et al to index a serde vec.

Expected:

        Ok(IbcBasicResponse::new()
            // ...
            .add_attributes(
                msg.token_ids
                    .into_iter()
                    .map(|token_id| ("token_id".to_string(), String::from(token_id)))
                    .collect::<Vec<(String, String)>>(),
            ))
            // "token_id" => "mytokenid1"
            // "token_id" => "mytokenid2"

Actual:

    Ok(IbcBasicResponse::new()
        .add_messages(messages)
        .add_attribute("method", "handle_packet_fail")
        .add_attribute("token_ids", format!("{:?}", message.token_ids)) // "[TokenId(\"mytokenid1\"),TokenId(\"mytokenid2\")]"

@towerkyoto towerkyoto added the ics-721 Possible protocol vulnerability label Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ics-721 Possible protocol vulnerability improv
Projects
None yet
Development

No branches or pull requests

2 participants