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

fix: override TxDecoder #1666

Merged
merged 4 commits into from
Apr 28, 2023
Merged

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Apr 26, 2023

Closes #1620
Closes #1665

Testing

  1. After this change, querying for an ordinary token transfer transaction still works.
  2. Before this change, querying for a tx that includes a MsgPayForBlobs would result in an error. After this change, it returns the transaction:
$ ./build/celestia-appd query tx ACF2D0D2B4F1EAAEF01AFC3F3EA72FB4CE3B2AB45F02C39BB868D58A1964A35F --home $DIR
code: 0
codespace: ""
data: 122A0A282F63656C65737469612E626C6F622E76312E4D7367506179466F72426C6F6273526573706F6E7365
events:
- attributes:
  - index: true
    key: c3BlbmRlcg==
    value: Y2VsZXN0aWExbmpnNXdtbDc1MnJreGQ3ZzgzemtjZWx4aGduZTdnNmFheHJtZGU=
  - index: true
    key: YW1vdW50
    value: MTAwMHV0aWE=
  type: coin_spent
- attributes:
  - index: true
    key: cmVjZWl2ZXI=
    value: Y2VsZXN0aWExN3hwZnZha20yYW1nOTYyeWxzNmY4NHoza2VsbDhjNWxwbmpzM3M=
  - index: true
    key: YW1vdW50
    value: MTAwMHV0aWE=
  type: coin_received
- attributes:
  - index: true
    key: cmVjaXBpZW50
    value: Y2VsZXN0aWExN3hwZnZha20yYW1nOTYyeWxzNmY4NHoza2VsbDhjNWxwbmpzM3M=
  - index: true
    key: c2VuZGVy
    value: Y2VsZXN0aWExbmpnNXdtbDc1MnJreGQ3ZzgzemtjZWx4aGduZTdnNmFheHJtZGU=
  - index: true
    key: YW1vdW50
    value: MTAwMHV0aWE=
  type: transfer
- attributes:
  - index: true
    key: c2VuZGVy
    value: Y2VsZXN0aWExbmpnNXdtbDc1MnJreGQ3ZzgzemtjZWx4aGduZTdnNmFheHJtZGU=
  type: message
- attributes:
  - index: true
    key: ZmVl
    value: MTAwMHV0aWE=
  - index: true
    key: ZmVlX3BheWVy
    value: Y2VsZXN0aWExbmpnNXdtbDc1MnJreGQ3ZzgzemtjZWx4aGduZTdnNmFheHJtZGU=
  type: tx
- attributes:
  - index: true
    key: YWNjX3NlcQ==
    value: Y2VsZXN0aWExbmpnNXdtbDc1MnJreGQ3ZzgzemtjZWx4aGduZTdnNmFheHJtZGUvMQ==
  type: tx
- attributes:
  - index: true
    key: c2lnbmF0dXJl
    value: bEFvLzFHNmNwNHN2YzZMM3lkaW8zRU1hQVJzbGFwWSt3VTE0dFdMdTg2cGtqR3piallLWC85T2M4cjJYc1NJdW1WbnJKQk9mMWlIS2tiV0lsR1ZMeGc9PQ==
  type: tx
- attributes:
  - index: true
    key: YWN0aW9u
    value: L2NlbGVzdGlhLmJsb2IudjEuTXNnUGF5Rm9yQmxvYnM=
  type: message
- attributes:
  - index: true
    key: YmxvYl9zaXplcw==
    value: WzEwMF0=
  - index: true
    key: bmFtZXNwYWNlcw==
    value: WyJBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBUzRGZjNPTkw0MW02MSJd
  - index: true
    key: c2lnbmVy
    value: ImNlbGVzdGlhMXZkamtjZXRudzM1a3p2dHdkZm5uMmFtZGRzbW4ydm5qZGR1eGdkbTg4cWVoNTZtcnY0azhzNnI4ZGVqbndlZWt2OXNoc3VuZHYzanNodDU2NHYi
  type: celestia.blob.v1.EventPayForBlobs
gas_used: "66666"
gas_wanted: "210000"
height: "2"
info: ""
logs:
- events:
  - attributes:
    - key: blob_sizes
      value: '[100]'
    - key: namespaces
      value: '["AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAS4Ff3ONL41m61"]'
    - key: signer
      value: '"celestia1vdjkcetnw35kzvtwdfnn2amddsmn2vnjdduxgdm88qeh56mrv4k8s6r8dejnweekv9shsundv3jsht564v"'
    type: celestia.blob.v1.EventPayForBlobs
  - attributes:
    - key: action
      value: /celestia.blob.v1.MsgPayForBlobs
    type: message
  log: ""
  msg_index: 0
raw_log: '[{"msg_index":0,"events":[{"type":"celestia.blob.v1.EventPayForBlobs","attributes":[{"key":"blob_sizes","value":"[100]"},{"key":"namespaces","value":"[\"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAS4Ff3ONL41m61\"]"},{"key":"signer","value":"\"celestia1vdjkcetnw35kzvtwdfnn2amddsmn2vnjdduxgdm88qeh56mrv4k8s6r8dejnweekv9shsundv3jsht564v\""}]},{"type":"message","attributes":[{"key":"action","value":"/celestia.blob.v1.MsgPayForBlobs"}]}]}]'
timestamp: "2023-04-26T20:41:47Z"
tx:
  '@type': /cosmos.tx.v1beta1.Tx
  auth_info:
    fee:
      amount:
      - amount: "1000"
        denom: utia
      gas_limit: "210000"
      granter: ""
      payer: ""
    signer_infos:
    - mode_info:
        single:
          mode: SIGN_MODE_DIRECT
      public_key:
        '@type': /cosmos.crypto.secp256k1.PubKey
        key: A+rTZhZk/Qd6zEiZBjPcvDF8WTn1v6eYXNe69U7r4l6P
      sequence: "1"
    tip: null
  body:
    extension_options: []
    memo: ""
    messages:
    - '@type': /celestia.blob.v1.MsgPayForBlobs
      blob_sizes:
      - 100
      namespaces:
      - AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAS4Ff3ONL41m61
      share_commitments:
      - GL2/4ivlkM3wQpfEoyqR4EiJ+mafCRKEFH5uA8HqnFQ=
      share_versions:
      - 0
      signer: celestia1njg5wml752rkxd7g83zkcelxhgne7g6aaxrmde
    non_critical_extension_options: []
    timeout_height: "0"
  signatures:
  - lAo/1G6cp4svc6L3ydio3EMaARslapY+wU14tWLu86pkjGzbjYKX/9Oc8r2XsSIumVnrJBOf1iHKkbWIlGVLxg==
txhash: ACF2D0D2B4F1EAAEF01AFC3F3EA72FB4CE3B2AB45F02C39BB868D58A1964A35F

@rootulp rootulp self-assigned this Apr 26, 2023
@rootulp rootulp added this to the Mainnet milestone Apr 26, 2023
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

imo we should also remove all the other instance of the index wrapper tx decoder and exclusively rely on the encoding config, and then unexport the index wrapper tx decoder

@rootulp rootulp added bug Something isn't working app labels Apr 27, 2023
@rootulp rootulp marked this pull request as ready for review April 27, 2023 16:40
@MSevey MSevey requested a review from a team April 27, 2023 18:09
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

LGTM! Wondering shouldn't this be marked as a breaking change?
Update: Skip my question, the prior behaviour was a bug and the current one is the correct one, so no breaking change!

@rootulp rootulp changed the title feat: override TxDecoder fix: override TxDecoder Apr 28, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #1666 (d569ea4) into main (3a9b939) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #1666   +/-   ##
=======================================
  Coverage   51.56%   51.56%           
=======================================
  Files          95       95           
  Lines        5954     5954           
=======================================
  Hits         3070     3070           
  Misses       2570     2570           
  Partials      314      314           
Impacted Files Coverage Δ
app/app.go 4.65% <0.00%> (ø)
test/util/testnode/read.go 0.00% <0.00%> (ø)

@cmwaters
Copy link
Contributor

Just FYI, I'm working on a prototype to have IndexWrapper become a sdk.Msg and be registered to the decoder in the blob module

@rootulp rootulp merged commit deb0a1e into celestiaorg:main Apr 28, 2023
24 checks passed
@rootulp rootulp deleted the rp/override-tx-decoder branch April 28, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the IndexWrappedTxDecoder universally Unable to parse PayForBlobs via CLI
5 participants