Repository navigation
Conversation
341855d to
32336ec
Compare
|
@paulgnz can you review it please? |
paulgnz
left a comment
There was a problem hiding this comment.
Looks good, thanks Blas. Checked it out and ran it locally:
tsc -bbuilds clean.- 19/19 tests pass on Node 18. (The boilerplate test takes ~6s, so it needs a longer mocha timeout than the default 2s.)
- All six actions match the live
eosio.msigABI on XPR mainnet, including theproposal_hashbinary extension onapproveandinvalidate. - The requested-signer dedupe by
actor@permissionfixes a real bug: the old code dropped a second required permission from the same account.
Two small things before merging:
msig approvelink: "View Proposal" uses the approver (authorization.actor) instead of the proposer, so it points at the wrong proposal page. It was already like that before this PR, but it's a one-word fix while you're in there:args.proposer.test/commands/network.test.ts: it now asserts"chain": "proton", which depends on whatever network is saved in the local config. It fails on any machine set toproton-test. Could you stub the config or just assert onCurrent Network:?
FYI, not from this PR: @oclif/test crashes on load under Node 22 (Cannot read properties of undefined (reading 'filename')), so the suite only runs on Node ≤20.
Happy to approve after 1 and 2.
|
Addressed both review comments: fixed the approve proposal link to use the proposer and made the network test independent of local config. @paulgnz |
acfc4ab to
45186af
Compare
|
Thanks! Both fixes look right. One catch: the new assertion in Adding test
.stub(network, 'transact', capture(transaction))
.stdout()
.command(['msig:approve', 'proposer', 'proposal', 'signer'])I'll approve once that's in. |
|
Added |
paulgnz
left a comment
There was a problem hiding this comment.
Approving. Thanks, the .stdout() fix did it.
Verified on a fresh checkout of 6c883e1 with Node 18:
tsc -bbuilds.- 19/19 tests pass (
mocha --require ts-node/register "test/**/*.test.ts"). - eslint is clean on all changed files.
approve,cancel,execandproposetake the same arguments as on master, so existing usage doesn't break.
Two things for a follow-up, both already on master and not caused by this PR:
npm testdoesn't run the suite. Current mocha ignorestest/mocha.opts, so the.tsfiles never load (ERR_UNKNOWN_FILE_EXTENSION). Moving those options into a.mocharc.json(withrequire: ts-node/register) should fix it.engines.nodesays>=14.0.0, but the code usesnode:imports, which need 16+. On very new Node (26)@oclif/testalso fails to load, so for now we should document Node 18/20 as the tested versions.
|
Nice addition of functionality and tests. This should clear one item in the TODO file. That being said, I've run this through Gemini 3.8 Flash (via Cursor) which has feedback I'm posting below. I understand that AI model output is not always on point, so please take a look and let me know what you think: Must Be Addressed in This Branch Before MergingThese are critical correctness bugs, protocol-level consensus constraints, and regressions that directly affect the code modified in this branch. If merged as-is, they will cause transactions to fail on-chain or degrade production operations. 1. Canonical Sorting of
|
149af68 to
87cb842
Compare
87cb842 to
8419426
Compare
|
Thanks for the detailed review, @squdgy . I went through all five points and addressed them in commit 8419426:
I added regression coverage for sorting, fallback resolution, hash normalization and validation, formatted errors, and the exec transaction URL. I also rebuilt the branch from Verification on Node 18.20.8:
The repository’s older oclif test stack still does not load under Node 26, so Node 18 is the runtime used for the full test run. |
Summary
Verification