Skip to content

Add origins in activity #373

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

Merged

Conversation

SmartLayer
Copy link

@SmartLayer SmartLayer commented Jul 5, 2020

As per TokenScript weekly meeting #40, developers should base their Activity A3 project on the files provided in ERCs directory. Notice that this is based on PR #365 which will be merged in a few weeks since αW is released with a fix

@SmartLayer SmartLayer changed the title Correct violation of rfc4912+add origins in activity Add origins in activity (based on Correct violation of rfc4912) Jul 5, 2020
@SmartLayer SmartLayer force-pushed the correct-violation-of-rfc4912+add-origins-in-activity branch from 0b100af to eb0aadc Compare July 5, 2020 02:48
…nt to fit JavaScript which uses the word 'amount'
@hboon
Copy link
Member

hboon commented Jul 6, 2020

Something to clear up before commenting on the JavaScript API changed to support this PR, instead of doing this:

web3.tokens.dataChanged = (oldTokens, updatedTokens, tokenIdCard) => {
    const currentTokenInstance = web3.tokens.data.currentInstance;
    document.getElementById(tokenIdCard).innerHTML = new Token(currentTokenInstance).render();
};

We should be doing this (i.e. not access web3.tokens.data.currentInstance):

web3.tokens.dataChanged = (oldTokens, updatedTokens, tokenIdCard) => {
    const currentTokenInstance = updatedTokens.currentInstance;
    document.getElementById(tokenIdCard).innerHTML = new Token(currentTokenInstance).render();
};

The latter assumes this:

{
  updatedTokens: {
    currentInstance: {
      tokenAttribute1: value,
      tokenAttribute2: value,
    }
  }
}

And for this PR

Let's just get rid of currentInstance once and for all, so web3 (under window) is not populated with currentInstance anymore:

{
  web3: {
    tokens: {
      data: {
        currentInstance: {
          tokenAttribute1: value,
          tokenAttribute2: value,
        }
      }
    }
  }
}

Instead, let's make the dataChanged handler work this way:

web3.tokens.dataChanged = (old, updated, tokenIdCard) => {
    const data = updated
    document.getElementById(tokenIdCard).innerHTML = new Token(data).render()
}

where updated (and old) looks like this:

updated = {
  token: {
    tokenAttribute1: value,
    tokenAttribute2: value,
  },
  card: {
    cardAttribute1: value,
    cardAttribute2: value,
  }
}

the developer can choose assign updated directly as props and thus access token attribute like this:

const tokenAttributeValue = this.props.token.tokenAttribute1
const cardAttributeValue = this.props.card.tokenAttribute1

And if they are sure the attribute names don't clash, this is easier to migrate to since only the dataChanged handler needs to be modified:

web3.tokens.dataChanged = (old, updated, tokenIdCard) => {
    const data = Object.assign({}, updated.token, updated.card)
    document.getElementById(tokenIdCard).innerHTML = new Token(data).render()
}

Then this is unchanged:

const attributeValue = this.props.tokenAttribute1 //or this.props.cardAttribute1

All of these assumes that we have at most 1 token, at most 1 card within each firing of dataChanged() Is that correct @colourful-land ?

@hboon
Copy link
Member

hboon commented Jul 6, 2020

So web3.tokens.dataChanged is the only place where we tuck things under web3. We can either change it now or next time. But it should be a very localised change (just that 1 line) when it happens.

@hboon
Copy link
Member

hboon commented Jul 13, 2020

@colourful-land have updated the JavaScript docs.

@hboon
Copy link
Member

hboon commented Jul 14, 2020

I suggest we preserve the value of updatedTokens.currentInstance for a while, i.e roughly this pseudo-code when injecting:

updatedTokens.currentInstance == updatedTokens.token + updatedTokens.card

so that the following JavaScript code in existing TokenScript files doesn't need to be updated immediately:

web3.tokens.dataChanged = (oldTokens, updatedTokens, tokenIdCard) => {
    const currentTokenInstance = updatedTokens.currentInstance;
    document.getElementById(tokenIdCard).innerHTML = new Token(currentTokenInstance).render();
};

cc @James-Sangalli

@hboon
Copy link
Member

hboon commented Jul 14, 2020

  • ping James Brown if the PR is good and gets merged so he is in the loop

@hboon
Copy link
Member

hboon commented Jul 15, 2020

Merge branch 'master' into correct-violation-of-rfc4912+add-origins-i…

git rebase master instead? :)

@SmartLayer
Copy link
Author

Merge branch 'master' into correct-violation-of-rfc4912+add-origins-i…

git rebase master instead? :)

Not sure if anyone has mod in local copy - also didn't like rebase

@SmartLayer
Copy link
Author

@James-Sangalli :

if rebased this message wouldn't appear in git log
@colourful-land Merge branch 'master' into correct-violation-of-rfc4912+add-origins-i…9d8061e

I don't know why it is a problem to be solved…

@JamesANZ
Copy link
Contributor

tokenAttribute1

@hboon I prefer this syntax for the web3 object as it separates them and prevents collisions (while also letting you know what is global and what is based on cards)

web3.tokens.dataChanged = (oldTokens, updated, tokenIdCard) => {
    document.getElementById(tokenIdCard).innerHTML = new Token(updated.token, updated.card).render();
};

constructor(token, card) {
        this.token = token;
        this.card = card;
}

@hboon
Copy link
Member

hboon commented Jul 15, 2020

tokenAttribute1

@hboon I prefer this syntax for the web3 object as it separates them and prevents collisions (while also letting you know what is global and what is based on cards)

web3.tokens.dataChanged = (oldTokens, updated, tokenIdCard) => {
    document.getElementById(tokenIdCard).innerHTML = new Token(updated.token, updated.card).render();
};

constructor(token, card) {
        this.token = token;
        this.card = card;
}

Up to you if that's a better way to write the JavaScript for the views. What you suggested is the same as the one I suggested for what the TokenScript engine exposes as an API, i.e there is updated.token and updated.card (and for the time being, hopefully briefly, the legacy updated.currentInstance) in here:

web3.tokens.dataChanged = (oldTokens, updated, tokenIdCard) => {
}

@SmartLayer SmartLayer changed the title Add origins in activity (based on Correct violation of rfc4912) Add origins in activity Jul 16, 2020
@JamesSmartCell
Copy link
Contributor

This PR seems to cover a diverse range of updates.

  1. The dataChanged method - this is how it was always supposed to be working, right? So far it's been a slow evolution toward this.
  2. The origins in activity is a good refactor and will make the parsing code more concise - so, it's acceptable :)

@SmartLayer SmartLayer marked this pull request as ready for review July 20, 2020 09:36
@SmartLayer SmartLayer merged commit 2bc5140 into master Jul 20, 2020
@SmartLayer SmartLayer deleted the correct-violation-of-rfc4912+add-origins-in-activity branch July 20, 2020 09:37
SmartLayer pushed a commit that referenced this pull request Jun 30, 2023
…+add-origins-in-activity

Add origins in activity
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.

4 participants