Skip to content

Feat/value prop trigger#984

Merged
Klakurka merged 4 commits intomasterfrom
feat/value-prop-trigger
Apr 16, 2025
Merged

Feat/value prop trigger#984
Klakurka merged 4 commits intomasterfrom
feat/value-prop-trigger

Conversation

@lissavxo
Copy link
Copy Markdown
Collaborator

Related to #977

Description

Added "value" variable to trigger variables.
Value will be an object with usd and cad attributes indicating the price in each fiat currency.

Test plan

Create a trigger and try to usa value variable.

@lissavxo lissavxo force-pushed the feat/value-prop-trigger branch from ac127af to feeefd4 Compare March 22, 2025 14:47
@lissavxo lissavxo requested review from Klakurka and chedieck and removed request for chedieck March 22, 2025 14:50
Copy link
Copy Markdown
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

We're likely going to have a lot of fiat currencies supported down the line so IMO we should just show the one they have set in their account config.

We can potentially make a task to allow them to grab a different currency if they want (or even multiple) but for now let's just use their set currency as that's what they'd be expecting.

@lissavxo lissavxo requested a review from Klakurka March 24, 2025 16:46
"txId": <txId>,
"...
}`}
"myButtonName": <buttonName>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This messes up the placeholder
image

rawMessage: string
inputAddresses: string[]
prices: Array<{
price: {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No need for all that. This is supposed to be simplified, so only take what is necessary, and for what I've gathered, it would be: value and quoteId.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Then alter getSimplifiedTransaction to take out these fields.

.replace('<opReturn>', opReturn)
.replace('<signature>', `${JSON.stringify(signature, undefined, 2)}`)
.replace('<inputAddresses>', `${JSON.stringify(postDataParameters.inputAddresses, undefined, 2)}`)
.replace('<value>', `${JSON.stringify(postDataParameters.value)}`)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why are you JSON.stringifing a string? Just do as it's done with the other strings, e.g.
`"${postDataParameters.address}"`

@lissavxo lissavxo requested a review from chedieck March 26, 2025 18:30
Copy link
Copy Markdown
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

I think this works well for a MVP.

Some future ideas could be including the currency name in the response & limiting the decimal cases to 2 digits.

@Klakurka Klakurka merged commit 5928796 into master Apr 16, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants