Page MenuHomePhabricator

[Cashtab] Update param parsing function to support opreturn param
ClosedPublic

Authored by bytesofman on Jan 4 2024, 00:21.

Details

Summary

Cashtab currently only supports the amount param.

Update parseAddressForParams function to also support the opreturn param.

Ref proof of concept D15032

Note: this diff does not yet implement the ability to create and broadcast txs using this param, although it does update a function currently implemented on the Send.js screen. This diff does not break existing amount functionality. For now, if a valid opreturn param is in a query string, it is ignored.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bytesofman published this revision for review.Jan 5 2024, 18:43
Fabien requested changes to this revision.Jan 5 2024, 20:16
Fabien added a subscriber: Fabien.
Fabien added inline comments.
cashtab/src/utils/__tests__/cashMethods.test.js
256 ↗(On Diff #43950)

shouldn't such a query be invalid because it doesn't have all the required fields (it's missing amount) ? You cannot do anything from that

cashtab/src/utils/cashMethods.js
1158 ↗(On Diff #43950)

That's not a good approach as you add more supported params.
You'd better extract all the keys and first check for double entries, then process if there is none. That will make it work with any other params just as well and you don't need a bool for everyone

This revision now requires changes to proceed.Jan 5 2024, 20:16
bytesofman marked an inline comment as done.

check for duplicates before processing

bytesofman added inline comments.
cashtab/src/utils/__tests__/cashMethods.test.js
256 ↗(On Diff #43950)

Cashtab runs every input address string through the parseAddressForParams function -- the function is where the logic for "does this address even have params?" exists.

So, a normal address with no params will return the above object.

Fabien requested changes to this revision.Jan 8 2024, 19:49

You have some commented code that is messing with the linter

cashtab/src/utils/cashMethods.js
1143 ↗(On Diff #43979)

Not sure if it's the most efficient way to do it but it's certainly a simple solution

This revision now requires changes to proceed.Jan 8 2024, 19:49
bytesofman marked an inline comment as done.

remove commented out code used in debugging tests

bytesofman added inline comments.
cashtab/src/utils/cashMethods.js
1143 ↗(On Diff #43979)

kind of a catch 22 here.

The available methods to check on duplicate keys also involve iterating over everything, which we already need to do to parse the individual params.

So, we can use bools -- which adds lots of flags, but catches duplicates during the iteration that needs to happen anyway.

Or we can do it up front -- which simplifies the code but does double up on iterations. I'm not sure if the new Set() approach is better or worse than alt approaches. I didn't want to have a function that is just doing a for loop before the for loop.

Fabien added inline comments.
cashtab/src/utils/cashMethods.js
1163–1169 ↗(On Diff #43980)

Style nit, same can be done below.

This revision is now accepted and ready to land.Jan 9 2024, 08:40