Revert ledger bug fix #56

clevinson · 2022-09-15T20:14:02Z

This PR reverts the components of #35 that were workarounds for the bug identified in Regen Ledger by @AmauryM (regen-network/regen-ledger#1480).

Yesterday we realized that Registry App does not in fact pass serialized sign bytes to Keplr, but rather the structured signDoc. This signDoc is used by Keplr for both amino serialization (to fwd to a hardware wallet) as well as for proto serialization (to encode in TxRaw). Because of this, there's no way to only strip out the top level {'type': 'regen.core.MsgSend', 'value': '...'} wrapper for amino serialization without forking the Keplr extension itself (something we don't want to do).

#35 still includes important work (all the amino converters are implemented, and have proper handling of default values, with several test cases written for most messages). This PR reverts the now unnecessary components of #35 that had to do with regen-network/regen-ledger#1480.

Note: Amino tests will fail in CI on this PR as there is not yet any dockerized infrastructure to fold the running of packages/api/e2e/regen-ledger it into CI.

$type fields

This reverts commit 81dea9a.

clevinson · 2022-10-03T03:53:21Z

So I've updated this PR to include an update to the tests so that we can now run tests against a locally configured regen node.

To summarize the changes in this PR are:

remove dependency on our CosmJS fork (this the main fix)
add new folder packages/api/e2e/regen-ledger with bash scripts for starting up a local regen node, and populating it with test data

To the run the tests locally you need to have two terminals.

In the first one:

Clone regen-ledger, checkout v4.1.2 and run make build
cd into packages/api/e2e/regen-ledger and make a copy of .env.sample into your own .env file
Edit .env file to point REGEN_BIN to your built binary from (1). This should be in build directory within the cloned repo.
Run source .env && ./bootstrap.sh from within packages/api/e2e/regen-ledger
This will setup a local single validator network, populate it with some initial ecocredit data required for the amino testing suite, and begin running the node

In the second terminal:

Run yarn test from within packages/api (make sure the tests you want to verify are not being skipped)

clevinson · 2022-10-03T03:54:49Z

I verified that all tests pass using the above steps. IMO we should try and dockerize this flow so that we can have these tests running in CI again. Is that something that @mhagel you or someone else on the interfaces team would feel comfortable taking over from here?

mhagel · 2022-10-04T19:28:43Z

I verified that all tests pass using the above steps. IMO we should try and dockerize this flow so that we can have these tests running in CI again. Is that something that @mhagel you or someone else on the interfaces team would feel comfortable taking over from here?

@wgwz Could you assist with this?

mhagel · 2022-10-04T22:32:21Z

confirmed tests pass locally.

mhagel · 2022-10-05T15:45:35Z

New issue to dockerize test flow #59

* chore: Upgrade @comjs/stargate and WIP on ecocredit amino converters * chore: Add modules * test: Add amino test * Remove non-essential content and clean up README * Improves for the README * Revert "Remove non-essential content and clean up README" This reverts commit 627f9a9. * Update dependencies, add MsgSend (#39) * feat: add MsgSend to aminomessages.ts * chore: isAminoMsgSend * chore: upgrade @cosmjs/stargate to 0.28.11 and include @cosmjs/proto-signing and @cosmjs/tendermint-rpc in dependencies as spec'd in https://github.com/cosmos/cosmjs/blob/main/packages/stargate/CUSTOM_PROTOBUF_CODECS.md - passing amino test for MsgSend * feat: AminoMsgSend_SendCredits * chore: add MsgSend and MsgCreateClass to customRegistry * wip: test with Secp256k1HdWallet * wip: amino tests * chore: add MsgCancel aminoType * chore: fix cancel_credits type * chore: back to v1 Credits * feat: ecocredit amino converters * chore: update amino dependency * feat: export all converters * feat: if field is falsey, return undefined in toAmino, omit nested (s) * chore: update ecocreditSendConverter with default handling * chore: lint and update basket and marketplace msgs * chore: successful MsgCreateClass test * feat: customMessageTypeRegistry * chore: note for future nested message types in send_amino.ts * chore: turn on marketplace and basket converters * chore: (temporary fix) delete nested msg types until ledger supports them * review: naming changes - customRegistry to regenRegistry * test: add createBasket test * Add patch file for cosmjs fork & fix basket test (#43) * add patch to cosmjs dependency for regen ledger amino support * add patch file * remove console log from patch file * remove patch-package from nested package.json * update package.json * update patch file * add fix for basket.MsgCreate test * chore: MsgPut and MsgTake tests * Update packages/api/src/tx/modules/ecocredit/basket/v1/create_amino.ts Co-authored-by: Mark Hagelberg <[email protected]> Co-authored-by: Mark Hagelberg <[email protected]> * Update proto files for regen (#44) * Clevinson/clean tests (#45) * udate MsgTake test * update MsgSend test * chore: revert x removal Co-authored-by: tyler <[email protected]> * test: amino tests for ecocredit core messages (#46) * test: add amin converter tests for ecocredit core * chore: move ethers to dev dependency * Update packages/api/src/tx/modules/ecocredit/v1/create_batch_amino.ts Co-authored-by: Mark Hagelberg <[email protected]> * chore: move util file * chore: add file to commit * chore: address review * fix: fromAmino dates use undefined Co-authored-by: Mark Hagelberg <[email protected]> * fix: msg send typo (#48) * fix: msg send and add test case * chore: re-enable test * fix: MsgCreateBatch fixes and tests (#50) * fix: msg send and add test case * fix: fixes and more tests * chore: remove test disable * Update cosmjs fork to pull from npm (#49) * update version * chore(release): Publish v0.4.0-alpha1.0 * add patch & patch-package to api sub package * chore(release): Publish v0.4.0-alpha1.1 * upgrade yarn and use yarn patch * bump version * chore(release): Publish v0.4.0-alpha1.2 * switch to npm published cosmjs-amino fork * chore(release): Publish v0.4.0-alpha1.3 * reset yarn to 1.x * rm patches * rm patches pt 2 * update yarn version * reset yarn.lock * Mark/marketplace amino (#47) * chore: - MsgSell converter fix and passing test - MsgBuyDirect converter change and (failing) test - MsgCancelSellOrder converter change and (failing) test * chore: omitDefault and longify in marketplace aminos * chore: consolidate util files * chore: fix MsgBuyDirect test * add more test cases * chore: amino test helpers file, fix boolean handling on BuyDirect * Update packages/api/src/tx/modules/ecocredit/converter-utils.ts Co-authored-by: Tyler <[email protected]> * chore: use omitDefault for boolean Co-authored-by: Cory Levinson <[email protected]> Co-authored-by: Tyler <[email protected]> * chore: update lint config, lint * Update yarn to 3.x (round2) (#53) * update yarn to stable (3.2.3) * add rimraf * chore: update deps * chore: move to dev deps * update workflow to run CI on all PRs irrespective of branch (#52) * trigger CI * add eslint to dev deps * add eslint to root dev deps * change eslint versino to 7.32.0 * align jest dependency with demo-app * move jest to a root resolution Co-authored-by: tyler <[email protected]> * update root & nested jest & ts-jest deps to consistent versions (#54) * chore: disable tests for CI - to avoid unnecessary redwood pollution, experimental release note (#55) * update changelog * chore(release): Publish v0.4.0-alpha1.4 * Revert ledger bug fix (#56) * reverts 849cf0a, adding back in nested $type fields * remove resolution for regen specific cosmjs fork * Revert "reverts 849cf0a, adding back in nested" This reverts commit 81dea9a. * test: add support for local testing Co-authored-by: Mark Hagelberg <[email protected]> * chore: disable integration tests to pass CI * chore: revert yarn release upgrade * chore: v0.4.0 * chore: restore messageTypeRegistry * chore: v0.4.0-alpha1.5 * chore: v0.4.0-alpha1.4 * chore(release): Publish v0.4.0-alpha1.5 * chore(release): Publish v0.4.0 Co-authored-by: Kyle Lawlor-Bagcal <[email protected]> Co-authored-by: Mark Hagelberg <[email protected]> Co-authored-by: Mark Hagelberg <[email protected]> Co-authored-by: Amaury <[email protected]> Co-authored-by: tyler <{ID}+{username}@users.noreply.github.com> Co-authored-by: Cory <[email protected]> Co-authored-by: tyler <[email protected]>

clevinson added 2 commits September 15, 2022 13:02

reverts 849cf0a, adding back in nested

81dea9a

$type fields

remove resolution for regen specific cosmjs fork

fda1764

mhagel approved these changes Sep 15, 2022

View reviewed changes

clevinson marked this pull request as ready for review September 15, 2022 22:45

clevinson mentioned this pull request Sep 16, 2022

Update tests to run against local network instead of redwood #57

Closed

clevinson added 2 commits September 23, 2022 16:21

Revert "reverts 849cf0a, adding back in nested"

227f1a0

This reverts commit 81dea9a.

test: add support for local testing

c61393f

clevinson requested a review from mhagel October 3, 2022 03:40

mhagel approved these changes Oct 3, 2022

View reviewed changes

chore: lint

87cfa74

mhagel merged commit eeef3a2 into marie/35-amino Oct 5, 2022

mhagel deleted the clevinson/revert-ledger-bug-override branch October 5, 2022 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert ledger bug fix #56

Revert ledger bug fix #56

clevinson commented Sep 15, 2022 •

edited

Loading

clevinson commented Oct 3, 2022

clevinson commented Oct 3, 2022

mhagel commented Oct 4, 2022

mhagel commented Oct 4, 2022

mhagel commented Oct 5, 2022

Revert ledger bug fix #56

Revert ledger bug fix #56

Conversation

clevinson commented Sep 15, 2022 • edited Loading

clevinson commented Oct 3, 2022

clevinson commented Oct 3, 2022

mhagel commented Oct 4, 2022

mhagel commented Oct 4, 2022

mhagel commented Oct 5, 2022

clevinson commented Sep 15, 2022 •

edited

Loading